Re: [PATCH] drm/i915: Fix remapped stride with CCS on ADL+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Dec 07, 2023 at 04:51:30PM +0200, Imre Deak wrote:
> On Tue, Dec 05, 2023 at 08:03:08PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On ADL+ the hardware automagically calculates the CCS AUX surface
> > stride from the main surface stride, so when remapping we can't
> > really play a lot of tricks with the main surface stride, or else
> > the AUX surface stride would get miscalculated and no longer
> > match the actual data layout in memory.
> > 
> > Supposedly we could remap in 256 main surface tile units
> > (AUX page(4096)/cachline(64)*4(4x1 main surface tiles per
> > AUX cacheline)=256 main surface tiles), but the extra complexity
> > is probably not worth the hassle.
> > 
> > So let's just make sure our mapping stride is calculated from
> > the full framebuffer stride (instead of the framebuffer width).
> > This way the stride we program into PLANE_STRIDE will be the
> > original framebuffer stride, and thus there will be no change
> > to the AUX stride/layout.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_fb.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index ab634a4c86d1..9f35bdce3eb8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -1509,8 +1509,20 @@ static u32 calc_plane_remap_info(const struct intel_framebuffer *fb, int color_p
> >  
> >  			size += remap_info->size;
> >  		} else {
> > -			unsigned int dst_stride = plane_view_dst_stride_tiles(fb, color_plane,
> > -									      remap_info->width);
> > +			unsigned int dst_stride;
> > +
> > +			/*
> > +			 * The hardware automagically calculates the CCS AUX surface
> > +			 * stride from the main surface stride so can't really remap a
> > +			 * smaller subset (unless we'd remap in whole AUX page units).
> > +			 */
> > +			if (intel_fb_needs_pot_stride_remap(fb) &&
> 
> This fix also applies at least to all !FLAT_CSS platforms. Since
> the stride remapping is disabled anyway on all platforms for CCS
> modifiers, the same should be done here as well?

We'll never get here for the ccs+!pot_stride_remap cases. So
I suppose it doesn't really matter how we express this.

But I think this check is the most correct one in the sense that
if we did want to come up with a way to do CCS remapping in the
!pot_stride_remap cases this simple approach wouldn't work anyway.
We'd end up here exactly because the original stride was too big
to begin with, so using to the original stride would solve
absolutely nothing.

> 
> > +			    intel_fb_is_ccs_modifier(fb->base.modifier))
> > +				dst_stride = remap_info->src_stride;
> > +			else
> > +				dst_stride = remap_info->width;
> > +
> > +			dst_stride = plane_view_dst_stride_tiles(fb, color_plane, dst_stride);
> >  
> >  			assign_chk_ovf(i915, remap_info->dst_stride, dst_stride);
> >  			color_plane_info->mapping_stride = dst_stride *
> > -- 
> > 2.41.0
> > 

-- 
Ville Syrjälä
Intel