diff mbox series

[v3,07/16] drm/i915: Fix PTE decode during initial plane readout

Message ID 20240116075636.6121-8-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: (stolen) memory region related fixes | expand

Commit Message

Ville Syrjälä Jan. 16, 2024, 7:56 a.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When multiple pipes are enabled by the BIOS we try to read out each
in turn. But we do the readout for the second only after the inherited
vma for the first has been rebound into its original place (and thus
the PTEs have been rewritten). Unlike the BIOS we set some high caching
bits in the PTE on MTL which confuses the readout for the second plane.
Filter out the non-address bits from the PTE value appropriately to
fix this.

I suppose it might also be possible that the BIOS would already set
some caching bits as well, in which case we'd run into this same
issue already for the first plane.

TODO:
- should abstract the PTE decoding to avoid details leaking all over
- should probably do the readout for all the planes before
  we touch anything (including the PTEs) so that we truly read
  out the BIOS state

Cc: Paz Zcharya <pazz@chromium.org>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nirmoy Das Jan. 16, 2024, 10:46 a.m. UTC | #1
On 1/16/2024 8:56 AM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When multiple pipes are enabled by the BIOS we try to read out each
> in turn. But we do the readout for the second only after the inherited
> vma for the first has been rebound into its original place (and thus
> the PTEs have been rewritten). Unlike the BIOS we set some high caching
> bits in the PTE on MTL which confuses the readout for the second plane.
> Filter out the non-address bits from the PTE value appropriately to
> fix this.
>
> I suppose it might also be possible that the BIOS would already set
> some caching bits as well, in which case we'd run into this same
> issue already for the first plane.
>
> TODO:
> - should abstract the PTE decoding to avoid details leaking all over
> - should probably do the readout for all the planes before
>    we touch anything (including the PTEs) so that we truly read
>    out the BIOS state
>
> Cc: Paz Zcharya <pazz@chromium.org>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index a55c09cbd0e4..ffc92b18fcf5 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -72,7 +72,7 @@ initial_plane_vma(struct drm_i915_private *i915,
>   			return NULL;
>   		}
>   
> -		phys_base = pte & I915_GTT_PAGE_MASK;
> +		phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
>   		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
>   
>   		/*
Paz Zcharya Jan. 30, 2024, 11:21 p.m. UTC | #2
On Tue, Jan 16, 2024 at 11:46:13AM +0100, Nirmoy Das wrote:
> 
> On 1/16/2024 8:56 AM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When multiple pipes are enabled by the BIOS we try to read out each
> > in turn. But we do the readout for the second only after the inherited
> > vma for the first has been rebound into its original place (and thus
> > the PTEs have been rewritten). Unlike the BIOS we set some high caching
> > bits in the PTE on MTL which confuses the readout for the second plane.
> > Filter out the non-address bits from the PTE value appropriately to
> > fix this.
> > 
> > I suppose it might also be possible that the BIOS would already set
> > some caching bits as well, in which case we'd run into this same
> > issue already for the first plane.
> > 
> > TODO:
> > - should abstract the PTE decoding to avoid details leaking all over
> > - should probably do the readout for all the planes before
> >    we touch anything (including the PTEs) so that we truly read
> >    out the BIOS state
> > 
> > Cc: Paz Zcharya <pazz@chromium.org>
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Acked-by: Nirmoy Das <nirmoy.das@intel.com>

Hi Ville,

Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130&rev=2
In addition, it solved the issue with the new GOP.

I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.

Tested-by: Paz Zcharya <pazz@chromium.org>

> > ---
> >   drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index a55c09cbd0e4..ffc92b18fcf5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -72,7 +72,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> >   			return NULL;
> >   		}
> > -		phys_base = pte & I915_GTT_PAGE_MASK;
> > +		phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
> >   		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> >   		/*
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index a55c09cbd0e4..ffc92b18fcf5 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -72,7 +72,7 @@  initial_plane_vma(struct drm_i915_private *i915,
 			return NULL;
 		}
 
-		phys_base = pte & I915_GTT_PAGE_MASK;
+		phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
 		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
 
 		/*