diff mbox series

[v2,08/15] drm/i915: Fix region start during initial plane readout

Message ID 20231215105929.29568-9-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ä Dec. 15, 2023, 10:59 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On MTL the stolen region starts at offset 8MiB from the start of
LMEMBAR. The dma addresses are thus also offset by 8MiB. However the
mm_node/etc. is zero based, and i915_pages_create_for_stolen() will
add the appropriate region.start into the sg dma address. So when
we do the readout we need to convert the dma address read from
the PTE to be zero based as well.

Note that currently we don't take this path on MTL, but we should
and thus this needs to be fixed. For lmem this works correctly
already as the lmem region.start==0.

While at it let's also make sure the address points to somewhere within
the memory region. We don't need to check the size as
i915_gem_object_create_region_at() should later fail if the object size
exceeds the region size.

Cc: Paz Zcharya <pazz@chromium.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_plane_initial.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Andrzej Hajda Dec. 18, 2023, 1 p.m. UTC | #1 
On 15.12.2023 11:59, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On MTL the stolen region starts at offset 8MiB from the start of
> LMEMBAR. The dma addresses are thus also offset by 8MiB. However the
> mm_node/etc. is zero based, and i915_pages_create_for_stolen() will
> add the appropriate region.start into the sg dma address. So when
> we do the readout we need to convert the dma address read from
> the PTE to be zero based as well.
> 
> Note that currently we don't take this path on MTL, but we should
> and thus this needs to be fixed. For lmem this works correctly
> already as the lmem region.start==0.
> 
> While at it let's also make sure the address points to somewhere within
> the memory region. We don't need to check the size as
> i915_gem_object_create_region_at() should later fail if the object size
> exceeds the region size.
> 
> Cc: Paz Zcharya <pazz@chromium.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_plane_initial.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index ffc92b18fcf5..db594ccf0323 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -79,16 +79,18 @@ initial_plane_vma(struct drm_i915_private *i915,
>   		 * We don't currently expect this to ever be placed in the
>   		 * stolen portion.
>   		 */
> -		if (phys_base >= resource_size(&mem->region)) {
> +		if (phys_base < mem->region.start || phys_base > mem->region.end) {

Maybe better:
phys_base + fb_size > mem->region.end" ?
Btw it seems redundant with later checks in 
i915_gem_object_create_region_at.
IMO at this moment we need only check if "phys_base -= 
mem->region.start" makes sense.

Regards
Andrzej


>   			drm_err(&i915->drm,
> -				"Initial plane programming using invalid range, phys_base=%pa\n",
> -				&phys_base);
> +				"Initial plane programming using invalid range, phys_base=%pa (%s [%pa-%pa])\n",
> +				&phys_base, mem->region.name, &mem->region.start, &mem->region.end);
>   			return NULL;
>   		}
>   
>   		drm_dbg(&i915->drm,
>   			"Using phys_base=%pa, based on initial plane programming\n",
>   			&phys_base);
> +
> +		phys_base -= mem->region.start;
>   	} else {
>   		phys_base = base;
>   		mem = i915->mm.stolen_region;
Ville Syrjälä Dec. 19, 2023, 12:01 a.m. UTC | #2 
On Mon, Dec 18, 2023 at 02:00:10PM +0100, Andrzej Hajda wrote:
> On 15.12.2023 11:59, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On MTL the stolen region starts at offset 8MiB from the start of
> > LMEMBAR. The dma addresses are thus also offset by 8MiB. However the
> > mm_node/etc. is zero based, and i915_pages_create_for_stolen() will
> > add the appropriate region.start into the sg dma address. So when
> > we do the readout we need to convert the dma address read from
> > the PTE to be zero based as well.
> > 
> > Note that currently we don't take this path on MTL, but we should
> > and thus this needs to be fixed. For lmem this works correctly
> > already as the lmem region.start==0.
> > 
> > While at it let's also make sure the address points to somewhere within
> > the memory region. We don't need to check the size as
> > i915_gem_object_create_region_at() should later fail if the object size
> > exceeds the region size.
> > 
> > Cc: Paz Zcharya <pazz@chromium.org>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_plane_initial.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index ffc92b18fcf5..db594ccf0323 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -79,16 +79,18 @@ initial_plane_vma(struct drm_i915_private *i915,
> >   		 * We don't currently expect this to ever be placed in the
> >   		 * stolen portion.
> >   		 */
> > -		if (phys_base >= resource_size(&mem->region)) {
> > +		if (phys_base < mem->region.start || phys_base > mem->region.end) {
> 
> Maybe better:
> phys_base + fb_size > mem->region.end" ?
> Btw it seems redundant with later checks in 
> i915_gem_object_create_region_at.
> IMO at this moment we need only check if "phys_base -= 
> mem->region.start" makes sense.

Yeah, I guess that alone would be sufficient. I left out the size
check exactly because I knew it would fail later, and making an
accurate check here (with page size rounding and whatnot) would
be tedious. But this should also be true when the start offset
is past the end of the region as well, so yeah I suppose I can
just drop the second check.


> 
> Regards
> Andrzej
> 
> 
> >   			drm_err(&i915->drm,
> > -				"Initial plane programming using invalid range, phys_base=%pa\n",
> > -				&phys_base);
> > +				"Initial plane programming using invalid range, phys_base=%pa (%s [%pa-%pa])\n",
> > +				&phys_base, mem->region.name, &mem->region.start, &mem->region.end);
> >   			return NULL;
> >   		}
> >   
> >   		drm_dbg(&i915->drm,
> >   			"Using phys_base=%pa, based on initial plane programming\n",
> >   			&phys_base);
> > +
> > +		phys_base -= mem->region.start;
> >   	} else {
> >   		phys_base = base;
> >   		mem = i915->mm.stolen_region;
Ville Syrjälä Jan. 12, 2024, 2:53 p.m. UTC | #3 
On Tue, Dec 19, 2023 at 02:01:15AM +0200, Ville Syrjälä wrote:
> On Mon, Dec 18, 2023 at 02:00:10PM +0100, Andrzej Hajda wrote:
> > On 15.12.2023 11:59, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On MTL the stolen region starts at offset 8MiB from the start of
> > > LMEMBAR. The dma addresses are thus also offset by 8MiB. However the
> > > mm_node/etc. is zero based, and i915_pages_create_for_stolen() will
> > > add the appropriate region.start into the sg dma address. So when
> > > we do the readout we need to convert the dma address read from
> > > the PTE to be zero based as well.
> > > 
> > > Note that currently we don't take this path on MTL, but we should
> > > and thus this needs to be fixed. For lmem this works correctly
> > > already as the lmem region.start==0.
> > > 
> > > While at it let's also make sure the address points to somewhere within
> > > the memory region. We don't need to check the size as
> > > i915_gem_object_create_region_at() should later fail if the object size
> > > exceeds the region size.
> > > 
> > > Cc: Paz Zcharya <pazz@chromium.org>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_plane_initial.c | 8 +++++---
> > >   1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > > index ffc92b18fcf5..db594ccf0323 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > > @@ -79,16 +79,18 @@ initial_plane_vma(struct drm_i915_private *i915,
> > >   		 * We don't currently expect this to ever be placed in the
> > >   		 * stolen portion.
> > >   		 */
> > > -		if (phys_base >= resource_size(&mem->region)) {
> > > +		if (phys_base < mem->region.start || phys_base > mem->region.end) {
> > 
> > Maybe better:
> > phys_base + fb_size > mem->region.end" ?
> > Btw it seems redundant with later checks in 
> > i915_gem_object_create_region_at.
> > IMO at this moment we need only check if "phys_base -= 
> > mem->region.start" makes sense.
> 
> Yeah, I guess that alone would be sufficient. I left out the size
> check exactly because I knew it would fail later, and making an
> accurate check here (with page size rounding and whatnot) would
> be tedious. But this should also be true when the start offset
> is past the end of the region as well, so yeah I suppose I can
> just drop the second check.

After further pondering I think I'm leaning towards keeping this
as is, just to give a slightly more obvious debug message.

> 
> 
> > 
> > Regards
> > Andrzej
> > 
> > 
> > >   			drm_err(&i915->drm,
> > > -				"Initial plane programming using invalid range, phys_base=%pa\n",
> > > -				&phys_base);
> > > +				"Initial plane programming using invalid range, phys_base=%pa (%s [%pa-%pa])\n",
> > > +				&phys_base, mem->region.name, &mem->region.start, &mem->region.end);
> > >   			return NULL;
> > >   		}
> > >   
> > >   		drm_dbg(&i915->drm,
> > >   			"Using phys_base=%pa, based on initial plane programming\n",
> > >   			&phys_base);
> > > +
> > > +		phys_base -= mem->region.start;
> > >   	} else {
> > >   		phys_base = base;
> > >   		mem = i915->mm.stolen_region;
> 
> -- 
> Ville Syrjälä
> Intel
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 ffc92b18fcf5..db594ccf0323 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -79,16 +79,18 @@  initial_plane_vma(struct drm_i915_private *i915,
 		 * We don't currently expect this to ever be placed in the
 		 * stolen portion.
 		 */
-		if (phys_base >= resource_size(&mem->region)) {
+		if (phys_base < mem->region.start || phys_base > mem->region.end) {
 			drm_err(&i915->drm,
-				"Initial plane programming using invalid range, phys_base=%pa\n",
-				&phys_base);
+				"Initial plane programming using invalid range, phys_base=%pa (%s [%pa-%pa])\n",
+				&phys_base, mem->region.name, &mem->region.start, &mem->region.end);
 			return NULL;
 		}
 
 		drm_dbg(&i915->drm,
 			"Using phys_base=%pa, based on initial plane programming\n",
 			&phys_base);
+
+		phys_base -= mem->region.start;
 	} else {
 		phys_base = base;
 		mem = i915->mm.stolen_region;