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

[Imre Deak <imre.deak@intel.com>]

On Thu, Dec 07, 2023 at 05:20:51PM +0200, Ville Syrjälä wrote:
> 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.

Ah right, I missed that point.

> 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.

Yes, in that case the AUX surface should be tweaked instead.
The patch looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> 
> > 
> > > +			    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