* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-17 23:01 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-17 23:01 UTC (permalink / raw)
To: Rodrigo Vivi
Cc: matthew.auld, Tvrtko Ursulin, Daniel Vetter, Subrata Banik,
dri-devel, linux-kernel, Sean Paul, Andrzej Hajda, Marcin Wojtas,
Drew Davenport, David Airlie, intel-gfx, Nirmoy Das
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
> >
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-17 23:01 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-17 23:01 UTC (permalink / raw)
To: Rodrigo Vivi
Cc: Andrzej Hajda, Subrata Banik, Tvrtko Ursulin, intel-gfx,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Marcin Wojtas,
Drew Davenport, Nirmoy Das
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
> >
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-17 23:01 ` Paz Zcharya
@ 2023-11-21 12:06 ` Andrzej Hajda
-1 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-21 12:06 UTC (permalink / raw)
To: Paz Zcharya, Rodrigo Vivi
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, linux-kernel, dri-devel,
Sean Paul, matthew.auld, Daniel Vetter, Marcin Wojtas,
Drew Davenport, David Airlie, Nirmoy Das
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
>>>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-21 12:06 ` Andrzej Hajda
0 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-21 12:06 UTC (permalink / raw)
To: Paz Zcharya, Rodrigo Vivi
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, linux-kernel, dri-devel,
Sean Paul, matthew.auld, Marcin Wojtas, Drew Davenport,
Nirmoy Das
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
>>>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-21 12:06 ` Andrzej Hajda
@ 2023-11-22 13:26 ` Andrzej Hajda
-1 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-22 13:26 UTC (permalink / raw)
To: Paz Zcharya, Rodrigo Vivi
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, linux-kernel, dri-devel,
Sean Paul, matthew.auld, Daniel Vetter, Marcin Wojtas,
Drew Davenport, David Airlie, Nirmoy Das
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
>>>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-22 13:26 ` Andrzej Hajda
0 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-22 13:26 UTC (permalink / raw)
To: Paz Zcharya, Rodrigo Vivi
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, linux-kernel, dri-devel,
Sean Paul, matthew.auld, Marcin Wojtas, Drew Davenport,
Nirmoy Das
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
>>>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-22 13:26 ` Andrzej Hajda
(?)
@ 2023-11-28 1:20 ` Paz Zcharya
-1 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 1:20 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Rodrigo Vivi, Drew Davenport, David Airlie, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 1:20 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 1:20 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Rodrigo Vivi, Subrata Banik, Tvrtko Ursulin, intel-gfx,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Marcin Wojtas, Drew Davenport, David Airlie, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 1:20 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 1:20 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Rodrigo Vivi,
Drew Davenport, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-22 13:26 ` Andrzej Hajda
(?)
@ 2023-11-28 3:47 ` Paz Zcharya
-1 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 3:47 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Rodrigo Vivi, Drew Davenport, David Airlie, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 3:47 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 3:47 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Rodrigo Vivi, Subrata Banik, Tvrtko Ursulin, intel-gfx,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Marcin Wojtas, Drew Davenport, David Airlie, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 3:47 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 3:47 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Rodrigo Vivi,
Drew Davenport, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-28 3:47 ` Paz Zcharya
(?)
@ 2023-11-28 11:12 ` Andrzej Hajda
-1 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-28 11:12 UTC (permalink / raw)
To: Paz Zcharya
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Rodrigo Vivi,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Marcin Wojtas, Drew Davenport, David Airlie, Nirmoy Das
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
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 11:12 ` Andrzej Hajda
0 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-28 11:12 UTC (permalink / raw)
To: Paz Zcharya
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Rodrigo Vivi, Drew Davenport, David Airlie, Nirmoy Das
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
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 11:12 ` Andrzej Hajda
0 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2023-11-28 11:12 UTC (permalink / raw)
To: Paz Zcharya
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Rodrigo Vivi,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Marcin Wojtas,
Drew Davenport, Nirmoy Das
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
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-28 11:12 ` Andrzej Hajda
(?)
@ 2023-11-28 11:19 ` Paz Zcharya
-1 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 11:19 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Rodrigo Vivi,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Marcin Wojtas, Drew Davenport, David Airlie, Nirmoy Das
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!
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 11:19 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 11:19 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Rodrigo Vivi, Drew Davenport, David Airlie, Nirmoy Das
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!
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-28 11:19 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-28 11:19 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Rodrigo Vivi,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Marcin Wojtas,
Drew Davenport, Nirmoy Das
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!
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
2023-11-28 11:12 ` Andrzej Hajda
(?)
@ 2023-11-30 16:24 ` Paz Zcharya
-1 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-30 16:24 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Rodrigo Vivi,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Marcin Wojtas, Drew Davenport, David Airlie, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-30 16:24 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-30 16:24 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Marcin Wojtas,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Daniel Vetter,
Rodrigo Vivi, Drew Davenport, David Airlie, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
@ 2023-11-30 16:24 ` Paz Zcharya
0 siblings, 0 replies; 29+ messages in thread
From: Paz Zcharya @ 2023-11-30 16:24 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Subrata Banik, Tvrtko Ursulin, intel-gfx, Rodrigo Vivi,
linux-kernel, dri-devel, Sean Paul, matthew.auld, Marcin Wojtas,
Drew Davenport, Nirmoy Das
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
^ permalink raw reply [flat|nested] 29+ messages in thread