[PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Paz Zcharya]

Fix the value of variable `phys_base` to be the relative offset in
stolen memory, and not the absolute offset of the GSM.

Currently, the value of `phys_base` is set to "Surface Base Address,"
which in the case of Meter Lake is 0xfc00_0000. This causes the
function `i915_gem_object_create_region_at` to fail in line 128, when
it attempts to verify that the range does not overflow:

if (range_overflows(offset, size, resource_size(&mem->region)))
      return ERR_PTR(-EINVAL);

where:
  offset = 0xfc000000
  size = 0x8ca000
  mem->region.end + 1 = 0x4400000
  mem->region.start = 0x800000
  resource_size(&mem->region) = 0x3c00000

call stack:
  i915_gem_object_create_region_at
  initial_plane_vma
  intel_alloc_initial_plane_obj
  intel_find_initial_plane_obj
  intel_crtc_initial_plane_config

Looking at the flow coming next, we see that `phys_base` is only used
once, in function `_i915_gem_object_stolen_init`, in the context of
the offset *in* the stolen memory. Combining that with an
examinination of the history of the file seems to indicate the
current value set is invalid.

call stack (functions using `phys_base`)
  _i915_gem_object_stolen_init
  __i915_gem_object_create_region
  i915_gem_object_create_region_at
  initial_plane_vma
  intel_alloc_initial_plane_obj
  intel_find_initial_plane_obj
  intel_crtc_initial_plane_config

[drm:_i915_gem_object_stolen_init] creating preallocated stolen
object: stolen_offset=0x0000000000000000, size=0x00000000008ca000

Signed-off-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..e696cb13756a 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
 			"Using phys_base=%pa, based on initial plane programming\n",
 			&phys_base);
 	} else {
-		phys_base = base;
+		phys_base = 0;
 		mem = i915->mm.stolen_region;
 	}
 
-- 
2.42.0.869.gea05f2083d-goog

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Rodrigo Vivi]

On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
> Fix the value of variable `phys_base` to be the relative offset in
> stolen memory, and not the absolute offset of the GSM.

to me it looks like the other way around. phys_base is the physical
base address for the frame_buffer. Setting it to zero doesn't seem
to make that relative. And also doesn't look right.

> 
> Currently, the value of `phys_base` is set to "Surface Base Address,"
> which in the case of Meter Lake is 0xfc00_0000.

I don't believe this is a fixed value. IIRC this comes from the register
set by video bios, where the idea is to reuse the fb that was used so
far.

With this in mind I don't understand how that could overflow. Maybe
the size of the stolen is not right? maybe the size? maybe different
memory region?

> This causes the
> function `i915_gem_object_create_region_at` to fail in line 128, when
> it attempts to verify that the range does not overflow:
> 
> if (range_overflows(offset, size, resource_size(&mem->region)))
>       return ERR_PTR(-EINVAL);
> 
> where:
>   offset = 0xfc000000
>   size = 0x8ca000
>   mem->region.end + 1 = 0x4400000
>   mem->region.start = 0x800000
>   resource_size(&mem->region) = 0x3c00000
> 
> call stack:
>   i915_gem_object_create_region_at
>   initial_plane_vma
>   intel_alloc_initial_plane_obj
>   intel_find_initial_plane_obj
>   intel_crtc_initial_plane_config
> 
> Looking at the flow coming next, we see that `phys_base` is only used
> once, in function `_i915_gem_object_stolen_init`, in the context of
> the offset *in* the stolen memory. Combining that with an
> examinination of the history of the file seems to indicate the
> current value set is invalid.
> 
> call stack (functions using `phys_base`)
>   _i915_gem_object_stolen_init
>   __i915_gem_object_create_region
>   i915_gem_object_create_region_at
>   initial_plane_vma
>   intel_alloc_initial_plane_obj
>   intel_find_initial_plane_obj
>   intel_crtc_initial_plane_config
> 
> [drm:_i915_gem_object_stolen_init] creating preallocated stolen
> object: stolen_offset=0x0000000000000000, size=0x00000000008ca000
> 
> Signed-off-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..e696cb13756a 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
>  			"Using phys_base=%pa, based on initial plane programming\n",
>  			&phys_base);
>  	} else {
> -		phys_base = base;
> +		phys_base = 0;
>  		mem = i915->mm.stolen_region;
>  	}
>  
> -- 
> 2.42.0.869.gea05f2083d-goog
> 

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Paz Zcharya]

On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
> > Fix the value of variable `phys_base` to be the relative offset in
> > stolen memory, and not the absolute offset of the GSM.
> 
> to me it looks like the other way around. phys_base is the physical
> base address for the frame_buffer. Setting it to zero doesn't seem
> to make that relative. And also doesn't look right.
>
> > 
> > Currently, the value of `phys_base` is set to "Surface Base Address,"
> > which in the case of Meter Lake is 0xfc00_0000.
> 
> I don't believe this is a fixed value. IIRC this comes from the register
> set by video bios, where the idea is to reuse the fb that was used so
> far.
> 
> With this in mind I don't understand how that could overflow. Maybe
> the size of the stolen is not right? maybe the size? maybe different
> memory region?
>

Hi Rodrigo, thanks for the great comments.

Apologies for using a wrong/confusing terminology. I think 'phys_base'
is supposed to be the offset in the GEM BO, where base (or
"Surface Base Address") is supposed to be the GTT offset.

Other than what I wrote before, I noticed that the function 'i915_vma_pin'
which calls to 'i915_gem_gtt_reserve' is the one that binds the right
address space in the GTT for that stolen region.

I see that in the function 'i915_vma_insert' (full call stack below),
where if (flags & PIN_OFFSET_FIXED), then when calling 'i915_gem_gtt_reserve'
we add an offset.

Specifically in MeteorLake, and specifically when using GOP driver, this
offset is equal to 0xfc00_0000. But as you mentioned, this is not strict.

The if statement always renders true because in the function
'initial_plane_vma' we always set
pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
where pinctl == flags (see file 'intel_plane_initial.c' line 145).

Call stack:
drm_mm_reserve_node
i915_gem_gtt_reserve
	i915_vma_insert
i915_vma_pin_ww
i915_vma_pin
initial_plane_vma
intel_alloc_initial_plane_obj
intel_find_initial_plane_obj

Therefore, I believe the variable 'phys_base' in the
function 'initial_plane_vma,' should be the the offset in the GEM BO
and not the GTT offset, and because the base is added later on
in the function 'i915_gem_gtt_reserve', this value should not be
equal to base and be 0.

Hope it makes more sense.

> > This causes the
> > function `i915_gem_object_create_region_at` to fail in line 128, when
> > it attempts to verify that the range does not overflow:
> > 
> > if (range_overflows(offset, size, resource_size(&mem->region)))
> >       return ERR_PTR(-EINVAL);
> > 
> > where:
> >   offset = 0xfc000000
> >   size = 0x8ca000
> >   mem->region.end + 1 = 0x4400000
> >   mem->region.start = 0x800000
> >   resource_size(&mem->region) = 0x3c00000
> > 
> > call stack:
> >   i915_gem_object_create_region_at
> >   initial_plane_vma
> >   intel_alloc_initial_plane_obj
> >   intel_find_initial_plane_obj
> >   intel_crtc_initial_plane_config
> > 
> > Looking at the flow coming next, we see that `phys_base` is only used
> > once, in function `_i915_gem_object_stolen_init`, in the context of
> > the offset *in* the stolen memory. Combining that with an
> > examinination of the history of the file seems to indicate the
> > current value set is invalid.
> > 
> > call stack (functions using `phys_base`)
> >   _i915_gem_object_stolen_init
> >   __i915_gem_object_create_region
> >   i915_gem_object_create_region_at
> >   initial_plane_vma
> >   intel_alloc_initial_plane_obj
> >   intel_find_initial_plane_obj
> >   intel_crtc_initial_plane_config
> > 
> > [drm:_i915_gem_object_stolen_init] creating preallocated stolen
> > object: stolen_offset=0x0000000000000000, size=0x00000000008ca000
> > 
> > Signed-off-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..e696cb13756a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> >  			"Using phys_base=%pa, based on initial plane programming\n",
> >  			&phys_base);
> >  	} else {
> > -		phys_base = base;
> > +		phys_base = 0;
> >  		mem = i915->mm.stolen_region;
> >  	}
> >  
> > -- 
> > 2.42.0.869.gea05f2083d-goog
> > 

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Andrzej Hajda]

On 18.11.2023 00:01, Paz Zcharya wrote:
> On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
>> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
>>> Fix the value of variable `phys_base` to be the relative offset in
>>> stolen memory, and not the absolute offset of the GSM.
>>
>> to me it looks like the other way around. phys_base is the physical
>> base address for the frame_buffer. Setting it to zero doesn't seem
>> to make that relative. And also doesn't look right.
>>
>>>
>>> Currently, the value of `phys_base` is set to "Surface Base Address,"
>>> which in the case of Meter Lake is 0xfc00_0000.
>>
>> I don't believe this is a fixed value. IIRC this comes from the register
>> set by video bios, where the idea is to reuse the fb that was used so
>> far.
>>
>> With this in mind I don't understand how that could overflow. Maybe
>> the size of the stolen is not right? maybe the size? maybe different
>> memory region?
>>
> 
> Hi Rodrigo, thanks for the great comments.
> 
> Apologies for using a wrong/confusing terminology. I think 'phys_base'
> is supposed to be the offset in the GEM BO, where base (or
> "Surface Base Address") is supposed to be the GTT offset.

Since base is taken from PLANE_SURF register it should be resolvable via 
GGTT to physical address pointing to actual framebuffer.
I couldn't find anything in the specs.
The simplest approach would be then do the same as in case of DGFX:
		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
		gen8_pte_t pte;

		gte += base / I915_GTT_PAGE_SIZE;

		pte = ioread64(gte);
		phys_base = pte & I915_GTT_PAGE_MASK;

Regards
Andrzej


> 
> Other than what I wrote before, I noticed that the function 'i915_vma_pin'
> which calls to 'i915_gem_gtt_reserve' is the one that binds the right
> address space in the GTT for that stolen region.
> 
> I see that in the function 'i915_vma_insert' (full call stack below),
> where if (flags & PIN_OFFSET_FIXED), then when calling 'i915_gem_gtt_reserve'
> we add an offset.
> 
> Specifically in MeteorLake, and specifically when using GOP driver, this
> offset is equal to 0xfc00_0000. But as you mentioned, this is not strict.
> 
> The if statement always renders true because in the function
> 'initial_plane_vma' we always set
> pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
> where pinctl == flags (see file 'intel_plane_initial.c' line 145).
> 
> Call stack:
> drm_mm_reserve_node
> i915_gem_gtt_reserve
> 	i915_vma_insert
> i915_vma_pin_ww
> i915_vma_pin
> initial_plane_vma
> intel_alloc_initial_plane_obj
> intel_find_initial_plane_obj
> 
> Therefore, I believe the variable 'phys_base' in the
> function 'initial_plane_vma,' should be the the offset in the GEM BO
> and not the GTT offset, and because the base is added later on
> in the function 'i915_gem_gtt_reserve', this value should not be
> equal to base and be 0.
> 
> Hope it makes more sense.
> 
>>> This causes the
>>> function `i915_gem_object_create_region_at` to fail in line 128, when
>>> it attempts to verify that the range does not overflow:
>>>
>>> if (range_overflows(offset, size, resource_size(&mem->region)))
>>>        return ERR_PTR(-EINVAL);
>>>
>>> where:
>>>    offset = 0xfc000000
>>>    size = 0x8ca000
>>>    mem->region.end + 1 = 0x4400000
>>>    mem->region.start = 0x800000
>>>    resource_size(&mem->region) = 0x3c00000
>>>
>>> call stack:
>>>    i915_gem_object_create_region_at
>>>    initial_plane_vma
>>>    intel_alloc_initial_plane_obj
>>>    intel_find_initial_plane_obj
>>>    intel_crtc_initial_plane_config
>>>
>>> Looking at the flow coming next, we see that `phys_base` is only used
>>> once, in function `_i915_gem_object_stolen_init`, in the context of
>>> the offset *in* the stolen memory. Combining that with an
>>> examinination of the history of the file seems to indicate the
>>> current value set is invalid.
>>>
>>> call stack (functions using `phys_base`)
>>>    _i915_gem_object_stolen_init
>>>    __i915_gem_object_create_region
>>>    i915_gem_object_create_region_at
>>>    initial_plane_vma
>>>    intel_alloc_initial_plane_obj
>>>    intel_find_initial_plane_obj
>>>    intel_crtc_initial_plane_config
>>>
>>> [drm:_i915_gem_object_stolen_init] creating preallocated stolen
>>> object: stolen_offset=0x0000000000000000, size=0x00000000008ca000
>>>
>>> Signed-off-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..e696cb13756a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>> @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
>>>   			"Using phys_base=%pa, based on initial plane programming\n",
>>>   			&phys_base);
>>>   	} else {
>>> -		phys_base = base;
>>> +		phys_base = 0;
>>>   		mem = i915->mm.stolen_region;
>>>   	}
>>>   
>>> -- 
>>> 2.42.0.869.gea05f2083d-goog
>>>

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Andrzej Hajda]

On 21.11.2023 13:06, Andrzej Hajda wrote:
> On 18.11.2023 00:01, Paz Zcharya wrote:
>> On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
>>> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
>>>> Fix the value of variable `phys_base` to be the relative offset in
>>>> stolen memory, and not the absolute offset of the GSM.
>>>
>>> to me it looks like the other way around. phys_base is the physical
>>> base address for the frame_buffer. Setting it to zero doesn't seem
>>> to make that relative. And also doesn't look right.
>>>
>>>>
>>>> Currently, the value of `phys_base` is set to "Surface Base Address,"
>>>> which in the case of Meter Lake is 0xfc00_0000.
>>>
>>> I don't believe this is a fixed value. IIRC this comes from the register
>>> set by video bios, where the idea is to reuse the fb that was used so
>>> far.
>>>
>>> With this in mind I don't understand how that could overflow. Maybe
>>> the size of the stolen is not right? maybe the size? maybe different
>>> memory region?
>>>
>>
>> Hi Rodrigo, thanks for the great comments.
>>
>> Apologies for using a wrong/confusing terminology. I think 'phys_base'
>> is supposed to be the offset in the GEM BO, where base (or
>> "Surface Base Address") is supposed to be the GTT offset.
> 
> Since base is taken from PLANE_SURF register it should be resolvable via 
> GGTT to physical address pointing to actual framebuffer.
> I couldn't find anything in the specs.

It was quite cryptic. I meant I have not found anything about assumption 
from commit history that for iGPU there should be 1:1 mapping, this is 
why there was an assignment "phys_base = base". Possibly the assumption 
is not valid anymore for MTL(?).
Without the assumption we need to check GGTT to determine phys address.

> The simplest approach would be then do the same as in case of DGFX:
>          gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
>          gen8_pte_t pte;
> 
>          gte += base / I915_GTT_PAGE_SIZE;
> 
>          pte = ioread64(gte);
>          phys_base = pte & I915_GTT_PAGE_MASK;
> 
> Regards
> Andrzej
> 
> 
>>
>> Other than what I wrote before, I noticed that the function 
>> 'i915_vma_pin'
>> which calls to 'i915_gem_gtt_reserve' is the one that binds the right
>> address space in the GTT for that stolen region.
>>
>> I see that in the function 'i915_vma_insert' (full call stack below),
>> where if (flags & PIN_OFFSET_FIXED), then when calling 
>> 'i915_gem_gtt_reserve'
>> we add an offset.
>>
>> Specifically in MeteorLake, and specifically when using GOP driver, this
>> offset is equal to 0xfc00_0000. But as you mentioned, this is not strict.
>>
>> The if statement always renders true because in the function
>> 'initial_plane_vma' we always set
>> pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
>> where pinctl == flags (see file 'intel_plane_initial.c' line 145).
>>
>> Call stack:
>> drm_mm_reserve_node
>> i915_gem_gtt_reserve
>>     i915_vma_insert
>> i915_vma_pin_ww
>> i915_vma_pin
>> initial_plane_vma
>> intel_alloc_initial_plane_obj
>> intel_find_initial_plane_obj
>>
>> Therefore, I believe the variable 'phys_base' in the
>> function 'initial_plane_vma,' should be the the offset in the GEM BO
>> and not the GTT offset, and because the base is added later on
>> in the function 'i915_gem_gtt_reserve', this value should not be
>> equal to base and be 0.
>>
>> Hope it makes more sense.
>>
>>>> This causes the
>>>> function `i915_gem_object_create_region_at` to fail in line 128, when
>>>> it attempts to verify that the range does not overflow:
>>>>
>>>> if (range_overflows(offset, size, resource_size(&mem->region)))
>>>>        return ERR_PTR(-EINVAL);
>>>>
>>>> where:
>>>>    offset = 0xfc000000
>>>>    size = 0x8ca000
>>>>    mem->region.end + 1 = 0x4400000
>>>>    mem->region.start = 0x800000
>>>>    resource_size(&mem->region) = 0x3c00000
>>>>
>>>> call stack:
>>>>    i915_gem_object_create_region_at
>>>>    initial_plane_vma
>>>>    intel_alloc_initial_plane_obj
>>>>    intel_find_initial_plane_obj
>>>>    intel_crtc_initial_plane_config
>>>>
>>>> Looking at the flow coming next, we see that `phys_base` is only used
>>>> once, in function `_i915_gem_object_stolen_init`, in the context of
>>>> the offset *in* the stolen memory. Combining that with an
>>>> examinination of the history of the file seems to indicate the
>>>> current value set is invalid.
>>>>
>>>> call stack (functions using `phys_base`)
>>>>    _i915_gem_object_stolen_init
>>>>    __i915_gem_object_create_region
>>>>    i915_gem_object_create_region_at
>>>>    initial_plane_vma
>>>>    intel_alloc_initial_plane_obj
>>>>    intel_find_initial_plane_obj
>>>>    intel_crtc_initial_plane_config
>>>>
>>>> [drm:_i915_gem_object_stolen_init] creating preallocated stolen
>>>> object: stolen_offset=0x0000000000000000, size=0x00000000008ca000
>>>>
>>>> Signed-off-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..e696cb13756a 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>>> @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
>>>>               "Using phys_base=%pa, based on initial plane 
>>>> programming\n",
>>>>               &phys_base);
>>>>       } else {
>>>> -        phys_base = base;
>>>> +        phys_base = 0;
>>>>           mem = i915->mm.stolen_region;
>>>>       }
>>>> -- 
>>>> 2.42.0.869.gea05f2083d-goog
>>>>
> 

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Paz Zcharya]

On Wed, Nov 22, 2023 at 02:26:55PM +0100, Andrzej Hajda wrote:
> 
> 
> On 21.11.2023 13:06, Andrzej Hajda wrote:
> > On 18.11.2023 00:01, Paz Zcharya wrote:
> > > On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
> > > > On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
> > > 
> > > Hi Rodrigo, thanks for the great comments.
> > > 
> > > Apologies for using a wrong/confusing terminology. I think 'phys_base'
> > > is supposed to be the offset in the GEM BO, where base (or
> > > "Surface Base Address") is supposed to be the GTT offset.
> > 
> > Since base is taken from PLANE_SURF register it should be resolvable via
> > GGTT to physical address pointing to actual framebuffer.
> > I couldn't find anything in the specs.
> 
> It was quite cryptic. I meant I have not found anything about assumption
> from commit history that for iGPU there should be 1:1 mapping, this is why
> there was an assignment "phys_base = base". Possibly the assumption is not
> valid anymore for MTL(?).
> Without the assumption we need to check GGTT to determine phys address.
> 
> > The simplest approach would be then do the same as in case of DGFX:
> >          gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> >          gen8_pte_t pte;
> > 
> >          gte += base / I915_GTT_PAGE_SIZE;
> > 
> >          pte = ioread64(gte);
> >          phys_base = pte & I915_GTT_PAGE_MASK;
> > 
> > Regards
> > Andrzej
Hey Andrzej,

Sorry for the late response. I was OOO :)
I tried using the code you mentioned. It translates (in the very specific
case of MTL + GOP driver) to phys_base == 0080_0000h. Unfortunately, it
results in a corrupted screen -- the framebuffer is filled with zeros.

It seems like `i915_vma_pin_ww` already reserves and binds the GEM BO to the
correct address space independently of the value of `phys_base`.
The only thing `phys_base` affects is the value of `stolen->start`
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/gem/i915_gem_stolen.c#L747

So it seems to me that the maybe `phys_base` is named incorrectly and that it
does not reflect the physical address, but the start offset of
i915->mm.stolen_region.

I'm happy to run more tests / debug further.
Do you have more ideas of things to try?


Many thanks,
Paz

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Paz Zcharya]

On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote:
>
> On 21.11.2023 13:06, Andrzej Hajda wrote:
> > On 18.11.2023 00:01, Paz Zcharya wrote:
> > > On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
> > > > On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
> > >
> > > Hi Rodrigo, thanks for the great comments.
> > >
> > > Apologies for using a wrong/confusing terminology. I think 'phys_base'
> > > is supposed to be the offset in the GEM BO, where base (or
> > > "Surface Base Address") is supposed to be the GTT offset.
> >
> > Since base is taken from PLANE_SURF register it should be resolvable via
> > GGTT to physical address pointing to actual framebuffer.
> > I couldn't find anything in the specs.
>
> It was quite cryptic. I meant I have not found anything about assumption
> from commit history that for iGPU there should be 1:1 mapping, this is why
> there was an assignment "phys_base = base". Possibly the assumption is not
> valid anymore for MTL(?).
> Without the assumption we need to check GGTT to determine phys address.
>
> > The simplest approach would be then do the same as in case of DGFX:
> >          gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> >          gen8_pte_t pte;
> >
> >          gte += base / I915_GTT_PAGE_SIZE;
> >
> >          pte = ioread64(gte);
> >          phys_base = pte & I915_GTT_PAGE_MASK;
> >
> > Regards
> > Andrzej

Hey Andrzej,

On a second thought, what do you think about something like

+               gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
+               gen8_pte_t pte;
+               gte += base / I915_GTT_PAGE_SIZE;
+               pte = ioread64(gte);
+               pte = pte & I915_GTT_PAGE_MASK;
+               phys_base = pte - i915->mm.stolen_region->region.start;

The only difference is the last line.

Based on what I wrote before, I think `phys_base` is named incorrectly and
that it does not reflect the physical address, but the start offset of
i915->mm.stolen_region. So if we offset the start value of the stolen
region, this code looks correct to me (and it also works on my
MeteorLake device).

What do you think?


Many thanks,
Paz

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Andrzej Hajda]

On 28.11.2023 04:47, Paz Zcharya wrote:
> 
> On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote:
>>
>> On 21.11.2023 13:06, Andrzej Hajda wrote:
>>> On 18.11.2023 00:01, Paz Zcharya wrote:
>>>> On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
>>>>> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
>>>>
>>>> Hi Rodrigo, thanks for the great comments.
>>>>
>>>> Apologies for using a wrong/confusing terminology. I think 'phys_base'
>>>> is supposed to be the offset in the GEM BO, where base (or
>>>> "Surface Base Address") is supposed to be the GTT offset.
>>>
>>> Since base is taken from PLANE_SURF register it should be resolvable via
>>> GGTT to physical address pointing to actual framebuffer.
>>> I couldn't find anything in the specs.
>>
>> It was quite cryptic. I meant I have not found anything about assumption
>> from commit history that for iGPU there should be 1:1 mapping, this is why
>> there was an assignment "phys_base = base". Possibly the assumption is not
>> valid anymore for MTL(?).
>> Without the assumption we need to check GGTT to determine phys address.
>>
>>> The simplest approach would be then do the same as in case of DGFX:
>>>           gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
>>>           gen8_pte_t pte;
>>>
>>>           gte += base / I915_GTT_PAGE_SIZE;
>>>
>>>           pte = ioread64(gte);
>>>           phys_base = pte & I915_GTT_PAGE_MASK;
>>>
>>> Regards
>>> Andrzej
> 
> Hey Andrzej,
> 
> On a second thought, what do you think about something like
> 
> +               gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> +               gen8_pte_t pte;
> +               gte += base / I915_GTT_PAGE_SIZE;
> +               pte = ioread64(gte);
> +               pte = pte & I915_GTT_PAGE_MASK;
> +               phys_base = pte - i915->mm.stolen_region->region.start;
> 
> The only difference is the last line.

Bingo :) It seems to be generic algorithm to get phys_base for all 
platforms:
- on older platforms stolen_region points to system memory which starts 
at 0,
- on DG2 it uses lmem region which starts at 0 as well,
- on MTL stolen_region points to stolen-local which starts at 0x800000.

So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced
with sth generic.
1. Find pte.
2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem = 
i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;

Regards
Andrzej


> 
> Based on what I wrote before, I think `phys_base` is named incorrectly and
> that it does not reflect the physical address, but the start offset of
> i915->mm.stolen_region. So if we offset the start value of the stolen
> region, this code looks correct to me (and it also works on my
> MeteorLake device).
> 
> What do you think?
> 
> 
> Many thanks,
> Paz
> 

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Paz Zcharya]

On Tue, Nov 28, 2023 at 12:12:08PM +0100, Andrzej Hajda wrote:
> On 28.11.2023 04:47, Paz Zcharya wrote:
> > 
> > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote:
> > 
> > Hey Andrzej,
> > 
> > On a second thought, what do you think about something like
> > 
> > +               gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > +               gen8_pte_t pte;
> > +               gte += base / I915_GTT_PAGE_SIZE;
> > +               pte = ioread64(gte);
> > +               pte = pte & I915_GTT_PAGE_MASK;
> > +               phys_base = pte - i915->mm.stolen_region->region.start;
> > 
> > The only difference is the last line.
> 
> Bingo :) It seems to be generic algorithm to get phys_base for all
> platforms:
> - on older platforms stolen_region points to system memory which starts at
> 0,
> - on DG2 it uses lmem region which starts at 0 as well,
> - on MTL stolen_region points to stolen-local which starts at 0x800000.
> 
> So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced
> with sth generic.
> 1. Find pte.
> 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
> i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
> 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
> 
> Regards
> Andrzej
> 
> 

Good stuff!! I'll work on this revision and resubmit.

Thank you so much Andrzej!

Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Paz Zcharya]

On Tue, Nov 28, 2023 at 12:12:08PM +0100, Andrzej Hajda wrote:
> On 28.11.2023 04:47, Paz Zcharya wrote:
> > 
> > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote:
> > > 
> > > On 21.11.2023 13:06, Andrzej Hajda wrote:
> > > 
> > > > The simplest approach would be then do the same as in case of DGFX:
> > > >           gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > > >           gen8_pte_t pte;
> > > > 
> > > >           gte += base / I915_GTT_PAGE_SIZE;
> > > > 
> > > >           pte = ioread64(gte);
> > > >           phys_base = pte & I915_GTT_PAGE_MASK;
> > > > 
> > > > Regards
> > > > Andrzej
> > 
> > Hey Andrzej,
> > 
> > On a second thought, what do you think about something like
> > 
> > +               gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > +               gen8_pte_t pte;
> > +               gte += base / I915_GTT_PAGE_SIZE;
> > +               pte = ioread64(gte);
> > +               pte = pte & I915_GTT_PAGE_MASK;
> > +               phys_base = pte - i915->mm.stolen_region->region.start;
> > 
> > The only difference is the last line.
> 
> Bingo :) It seems to be generic algorithm to get phys_base for all
> platforms:
> - on older platforms stolen_region points to system memory which starts at
> 0,
> - on DG2 it uses lmem region which starts at 0 as well,
> - on MTL stolen_region points to stolen-local which starts at 0x800000.
> 
> So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced
> with sth generic.
> 1. Find pte.
> 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
> i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
> 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
> 
> Regards
> Andrzej
> 
> 

Hey Andrzej,

I uploaded https://patchwork.freedesktop.org/series/127130/ based on
algorithm. Please take a look and let me know if you'd like me to change
anything.

Really appreciate all of your help!


Best,
Paz