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@xxxxxxxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
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
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
