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

Jocelyn Falempe posted 10 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v10 03/10] drm/i915/display/i9xx: Add a disable_tiling() for i9xx planes
Posted by Jocelyn Falempe 3 months, 3 weeks ago
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);
+	}
+}
+
 struct intel_plane *
 intel_primary_plane_create(struct intel_display *display, enum pipe pipe)
 {
@@ -1047,6 +1068,8 @@ intel_primary_plane_create(struct intel_display *display, enum pipe pipe)
 		}
 	}
 
+	plane->disable_tiling = i9xx_disable_tiling;
+
 	modifiers = intel_fb_plane_get_modifiers(display, INTEL_PLANE_CAP_TILING_X);
 
 	if (DISPLAY_VER(display) >= 5 || display->platform.g4x)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index ed4d743fc7c5..3654d88e9c5f 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1517,6 +1517,8 @@ struct intel_plane {
 			   bool async_flip);
 	void (*enable_flip_done)(struct intel_plane *plane);
 	void (*disable_flip_done)(struct intel_plane *plane);
+	/* For drm_panic */
+	void (*disable_tiling)(struct intel_plane *plane);
 };
 
 #define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
-- 
2.49.0

Re: [PATCH v10 03/10] drm/i915/display/i9xx: Add a disable_tiling() for i9xx planes
Posted by Ville Syrjälä 2 months, 3 weeks ago
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
- 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
Re: [PATCH v10 03/10] drm/i915/display/i9xx: Add a disable_tiling() for i9xx planes
Posted by Jocelyn Falempe 2 months, 1 week ago
On 19/07/2025 20:23, 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 :(

Sorry for that. I replied to that thread, but I didn't get answer [1]

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

I agree that going from one tiling format to another is not safe. But 
from my understanding, going from tiling to linear should be possible.
Do you have an example, where the stride/alignment/watermark requirement 
in tiled would be incompatible in Linear (for the same resolution)?

> - this completely fails to account for the TILEOFF vs. LINOFF stuff

Pardon my ignorance, can you explain what it is, and how it can break or 
make the output unreadable?

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

Regarding the tiling format, I usually test on hardware to check that 
the image is correct. But I have only a few of them, and as the format 
is platform dependent, and sometime also depends on the memory 
configuration. For me it looks very hard to get it right.
I've done it only for Y-tile and 4-tile, but only when DPT is enabled 
(which means it's only the few latest generations).

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

No, I didn't write igts for this yet. I test by triggering a kernel 
panic, as it's the only way to make sure it works.
Also I didn't consider rotation yet, I think if the panic screen is not 
rotated, it's still useful.

> 
> /me off to summer vacation. Good luck
> 

Sorry for that, my goal is just to have drm panic working on intel GPU.
Enjoy your vacation, and let's find a solution when you're back.

[1] 
https://lore.kernel.org/intel-gfx/72fa1da6-caaa-41c9-aef1-4e780bde6acf@redhat.com/

Best regards,

-- 

Jocelyn

Re: [PATCH v10 03/10] drm/i915/display/i9xx: Add a disable_tiling() for i9xx planes
Posted by Ville Syrjälä 2 months, 3 weeks ago
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