| Message ID | 20190923102935.5860-4-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Gen12 E2E compression | expand |
On Mon, Sep 23, 2019 at 03:29:29AM -0700, Dhinakaran Pandiyan wrote: > Easier to read if all the alignment changes are in one place and contained > within a function. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 31 ++++++++++---------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index a94d145dd048..c437f00c2072 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -2551,7 +2551,22 @@ intel_fb_stride_alignment(const struct drm_framebuffer *fb, int color_plane) > else > return 64; > } else { > - return intel_tile_width_bytes(fb, color_plane); > + u32 tile_width = intel_tile_width_bytes(fb, color_plane); > + > + /* > + * Display WA #0531: skl,bxt,kbl,glk > + * > + * Render decompression and plane width > 3840 > + * combined with horizontal panning requires the > + * plane stride to be a multiple of 4. We'll just > + * require the entire fb to accommodate that to avoid > + * potential runtime errors at plane configuration time. > + */ > + if (IS_GEN(dev_priv, 9) && is_ccs_modifier(fb->modifier) && > + color_plane == 0 && fb->width > 3840) > + tile_width *= 4; I realize you're only moving this, but I find this workaround description confusing since the wording is somewhat ambiguous as to whether it's expecting the plane stride to be a multiple of 4 bytes or 4 tiles. On casual read, I think most people would assume that we're talking about bytes here. Only once you realize that the PLANE_STRIDE register itself gets programmed in units of tile width does the wording here become clear. Maybe we could clarify the comment while moving it? Also it might be slightly more clear to do a "return tile_width * 4" here instead of modifying tile_width since that's a bit more intuitive description of what we're trying to do. Either way, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > + > + return tile_width; > } > } > > @@ -15705,20 +15720,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > } > > stride_alignment = intel_fb_stride_alignment(fb, i); > - > - /* > - * Display WA #0531: skl,bxt,kbl,glk > - * > - * Render decompression and plane width > 3840 > - * combined with horizontal panning requires the > - * plane stride to be a multiple of 4. We'll just > - * require the entire fb to accommodate that to avoid > - * potential runtime errors at plane configuration time. > - */ > - if (IS_GEN(dev_priv, 9) && i == 0 && fb->width > 3840 && > - is_ccs_modifier(fb->modifier)) > - stride_alignment *= 4; > - > if (fb->pitches[i] & (stride_alignment - 1)) { > DRM_DEBUG_KMS("plane %d pitch (%d) must be at least %u byte aligned\n", > i, fb->pitches[i], stride_alignment); > -- > 2.17.1 >
On Wed, 2019-10-02 at 15:29 -0700, Matt Roper wrote: > On Mon, Sep 23, 2019 at 03:29:29AM -0700, Dhinakaran Pandiyan wrote: > > Easier to read if all the alignment changes are in one place and contained > > within a function. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 31 ++++++++++---------- > > 1 file changed, 16 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index a94d145dd048..c437f00c2072 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -2551,7 +2551,22 @@ intel_fb_stride_alignment(const struct drm_framebuffer *fb, int > > color_plane) > > else > > return 64; > > } else { > > - return intel_tile_width_bytes(fb, color_plane); > > + u32 tile_width = intel_tile_width_bytes(fb, color_plane); > > + > > + /* > > + * Display WA #0531: skl,bxt,kbl,glk > > + * > > + * Render decompression and plane width > 3840 > > + * combined with horizontal panning requires the > > + * plane stride to be a multiple of 4. We'll just > > + * require the entire fb to accommodate that to avoid > > + * potential runtime errors at plane configuration time. > > + */ > > + if (IS_GEN(dev_priv, 9) && is_ccs_modifier(fb->modifier) && > > + color_plane == 0 && fb->width > 3840) > > + tile_width *= 4; > > I realize you're only moving this, but I find this workaround > description confusing since the wording is somewhat ambiguous as to > whether it's expecting the plane stride to be a multiple of 4 bytes or 4 > tiles. On casual read, I think most people would assume that we're > talking about bytes here. Only once you realize that the PLANE_STRIDE > register itself gets programmed in units of tile width does the wording > here become clear. Maybe we could clarify the comment while moving it? I remember wanting to rewrite that comment for the exact reason, but forgot to do so. Thanks for the review, I'll fix it. > > Also it might be slightly more clear to do a "return tile_width * 4" > here instead of modifying tile_width since that's a bit more intuitive > description of what we're trying to do. > > Either way, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > Matt > > > > + > > + return tile_width; > > } > > } > > > > @@ -15705,20 +15720,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > > } > > > > stride_alignment = intel_fb_stride_alignment(fb, i); > > - > > - /* > > - * Display WA #0531: skl,bxt,kbl,glk > > - * > > - * Render decompression and plane width > 3840 > > - * combined with horizontal panning requires the > > - * plane stride to be a multiple of 4. We'll just > > - * require the entire fb to accommodate that to avoid > > - * potential runtime errors at plane configuration time. > > - */ > > - if (IS_GEN(dev_priv, 9) && i == 0 && fb->width > 3840 && > > - is_ccs_modifier(fb->modifier)) > > - stride_alignment *= 4; > > - > > if (fb->pitches[i] & (stride_alignment - 1)) { > > DRM_DEBUG_KMS("plane %d pitch (%d) must be at least %u byte aligned\n", > > i, fb->pitches[i], stride_alignment); > > -- > > 2.17.1 > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index a94d145dd048..c437f00c2072 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -2551,7 +2551,22 @@ intel_fb_stride_alignment(const struct drm_framebuffer *fb, int color_plane) else return 64; } else { - return intel_tile_width_bytes(fb, color_plane); + u32 tile_width = intel_tile_width_bytes(fb, color_plane); + + /* + * Display WA #0531: skl,bxt,kbl,glk + * + * Render decompression and plane width > 3840 + * combined with horizontal panning requires the + * plane stride to be a multiple of 4. We'll just + * require the entire fb to accommodate that to avoid + * potential runtime errors at plane configuration time. + */ + if (IS_GEN(dev_priv, 9) && is_ccs_modifier(fb->modifier) && + color_plane == 0 && fb->width > 3840) + tile_width *= 4; + + return tile_width; } } @@ -15705,20 +15720,6 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, } stride_alignment = intel_fb_stride_alignment(fb, i); - - /* - * Display WA #0531: skl,bxt,kbl,glk - * - * Render decompression and plane width > 3840 - * combined with horizontal panning requires the - * plane stride to be a multiple of 4. We'll just - * require the entire fb to accommodate that to avoid - * potential runtime errors at plane configuration time. - */ - if (IS_GEN(dev_priv, 9) && i == 0 && fb->width > 3840 && - is_ccs_modifier(fb->modifier)) - stride_alignment *= 4; - if (fb->pitches[i] & (stride_alignment - 1)) { DRM_DEBUG_KMS("plane %d pitch (%d) must be at least %u byte aligned\n", i, fb->pitches[i], stride_alignment);
Easier to read if all the alignment changes are in one place and contained within a function. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 31 ++++++++++---------- 1 file changed, 16 insertions(+), 15 deletions(-)