Re: [PATCH v10 03/10] drm/i915/display/i9xx: Add a disable_tiling() for i9xx planes

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

On Sat, Jul 19, 2025 at 09:23:04PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 18, 2025 at 11:31:21AM +0200, Jocelyn Falempe wrote:
> > drm_panic draws in linear framebuffer, so it's easier to re-use the
> > current framebuffer, and disable tiling in the panic handler, to show
> > the panic screen.
> > This assumes that the alignment restriction is always smaller in
> > linear than in tiled.
> > It also assumes that the linear framebuffer size is always smaller
> > than the tiled.
> > 
> > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> > ---
> > 
> > v7:
> >  * Reword commit message about alignment/size when disabling tiling (Ville Syrjälä)
> > 
> >  drivers/gpu/drm/i915/display/i9xx_plane.c     | 23 +++++++++++++++++++
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index 8f15333a4b07..0807fae12450 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -905,6 +905,27 @@ static const struct drm_plane_funcs i8xx_plane_funcs = {
> >  	.format_mod_supported_async = intel_plane_format_mod_supported_async,
> >  };
> >  
> > +static void i9xx_disable_tiling(struct intel_plane *plane)
> > +{
> > +	struct intel_display *display = to_intel_display(plane);
> > +	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> > +	u32 dspcntr;
> > +	u32 reg;
> > +
> > +	dspcntr = intel_de_read_fw(display, DSPCNTR(display, i9xx_plane));
> > +	dspcntr &= ~DISP_TILED;
> > +	intel_de_write_fw(display, DSPCNTR(display, i9xx_plane), dspcntr);
> > +
> > +	if (DISPLAY_VER(display) >= 4) {
> > +		reg = intel_de_read_fw(display, DSPSURF(display, i9xx_plane));
> > +		intel_de_write_fw(display, DSPSURF(display, i9xx_plane), reg);
> > +
> > +	} else {
> > +		reg = intel_de_read_fw(display, DSPADDR(display, i9xx_plane));
> > +		intel_de_write_fw(display, DSPADDR(display, i9xx_plane), reg);
> > +	}
> > +}
> 
> I thought I already shot this down before, but apparently this
> got merged now :(
> 
> Just to reiterate why we don't want these 'disable tiling' hacks:
> - different tiling formats have different stride/alignment/watermark
>   requirements so one can't safely change from one tiling to another
> - this completely fails to account for the TILEOFF vs. LINOFF stuff

Oh yeah, and rotation support of course is one really big difference
between different tiling formats.

> - etc.
> 
> So IMO these hacks must be removed and instead the code must learn how
> to propetly write the tiled data. igt has all the code for that btw
> (twice over IIRC) so shouldn't be that hard.
> 
> I suppose the only hack we need to keep is to disable compression,
> mainly because (IIRC) on flat CCS systems the CPU doesn't have access
> to the AUX data to clear it manually.
> 
> I also wonder if there are actual igts for this? I think what is needed
> is a test that sets random things (different panning, rotation, pixel
> foramts, etc.) and triggers the dumper. Not quite sure how the test
> could validate that the output is correct though. CRCs might be a bit
> tricky since you need an identical reference image.
> 
> /me off to summer vacation. Good luck
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel