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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
>
> >
> > > + 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
