[Intel-gfx] [PATCH] drm/i915/display: Add smem fallback allocation for dpt
Juha-Pekka Heikkila
juhapekka.heikkila at gmail.com
Wed Mar 23 15:09:32 UTC 2022
On 22.3.2022 17.53, Matthew Auld wrote:
> On Tue, 22 Mar 2022 at 12:06, Juha-Pekka Heikkila
> <juhapekka.heikkila at gmail.com> wrote:
>>
>> On 22.3.2022 12.45, Matthew Auld wrote:
>>> On Mon, 21 Mar 2022 at 18:36, Juha-Pekka Heikkila
>>> <juhapekka.heikkila at gmail.com> wrote:
>>>>
>>>> On 21.3.2022 14.29, Matthew Auld wrote:
>>>>> On Fri, 18 Mar 2022 at 09:22, Juha-Pekka Heikkila
>>>>> <juhapekka.heikkila at gmail.com> wrote:
>>>>>>
>>>>>> On 17.3.2022 13.55, Matthew Auld wrote:
>>>>>>> On Wed, 16 Mar 2022 at 22:23, Juha-Pekka Heikkila
>>>>>>> <juhapekka.heikkila at gmail.com> wrote:
>>>>>>>>
>>>>>>>> Add fallback smem allocation for dpt if stolen memory
>>>>>>>> allocation failed.
>>>>>>>>
>>>>>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/i915/display/intel_dpt.c | 18 ++++++++++++++----
>>>>>>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
>>>>>>>> index fb0e7e79e0cd..c8b66433d4db 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
>>>>>>>> @@ -10,6 +10,7 @@
>>>>>>>> #include "intel_display_types.h"
>>>>>>>> #include "intel_dpt.h"
>>>>>>>> #include "intel_fb.h"
>>>>>>>> +#include "gem/i915_gem_internal.h"
>>>>>>>
>>>>>>> Nit: these should be kept sorted
>>>>>>>
>>>>>>>>
>>>>>>>> struct i915_dpt {
>>>>>>>> struct i915_address_space vm;
>>>>>>>> @@ -128,6 +129,10 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>>>>>>>> void __iomem *iomem;
>>>>>>>> struct i915_gem_ww_ctx ww;
>>>>>>>> int err;
>>>>>>>> + u64 pin_flags = 0;
>>>>>>>> +
>>>>>>>> + if (i915_gem_object_is_stolen(dpt->obj))
>>>>>>>> + pin_flags |= PIN_MAPPABLE; /* for i915_vma_pin_iomap(stolen) */
>>>>>>>>
>>>>>>>> wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>>>>>>>> atomic_inc(&i915->gpu_error.pending_fb_pin);
>>>>>>>> @@ -138,7 +143,7 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>>>>>>>> continue;
>>>>>>>>
>>>>>>>> vma = i915_gem_object_ggtt_pin_ww(dpt->obj, &ww, NULL, 0, 4096,
>>>>>>>> - HAS_LMEM(i915) ? 0 : PIN_MAPPABLE);
>>>>>>>> + pin_flags);
>>>>>>>> if (IS_ERR(vma)) {
>>>>>>>> err = PTR_ERR(vma);
>>>>>>>> continue;
>>>>>>>> @@ -248,10 +253,15 @@ intel_dpt_create(struct intel_framebuffer *fb)
>>>>>>>>
>>>>>>>> size = round_up(size * sizeof(gen8_pte_t), I915_GTT_PAGE_SIZE);
>>>>>>>>
>>>>>>>> - if (HAS_LMEM(i915))
>>>>>>>> - dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
>>>>>>>> - else
>>>>>>>> + dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
>>>>>>>> + if (IS_ERR(dpt_obj) && i915_ggtt_has_aperture(to_gt(i915)->ggtt))
>>>>>>>> dpt_obj = i915_gem_object_create_stolen(i915, size);
>>>>>>>> + if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
>>>>>>>> + drm_dbg_kms(&i915->drm, "fb: [FB:%d] Allocating dpt from smem\n",
>>>>>>>> + fb->base.base.id);
>>>>>>>> +
>>>>>>>> + dpt_obj = i915_gem_object_create_internal(i915, size);
>>>>>>>
>>>>>>> Looks like we are missing some prerequisite patch to be able to
>>>>>>> directly map such memory in vma_pin_iomap?
>>>>>>
>>>>>> For these functions I'm more like a consumer, I was following
>>>>>> suggestions from Chris on this. Is there something extra that should be
>>>>>> considered in this regard when use it like this?
>>>>>
>>>>> AFAICT this will trigger the WARN_ON() in vma_pin_iomap() if we
>>>>> fallback to create_internal(), since the object is now not lmem and is
>>>>> also not map_and_fenceable(i.e PIN_MAPPABLE).
>>>>
>>>> This shouldn't affect case when dpt allocation from lmem failed, it is
>>>> expected to go to "return ERR_CAST(dpt_obj);" below these comments. On
>>>> situation when allocating lmem and stolen failed on next "if" I added
>>>> !HAS_LMEM(i915) to handle situation with lmem. Though, when I was
>>>> originally trying this patch without limiting lmem case I remember with
>>>> dg2 I got black screen but I don't remember seeing WARN_ON() in logs.
>>>>
>>>>>
>>>>> The other issue is that we need some way of CPU mapping this type of
>>>>> object, like with calling i915_gem_object_pin_map() inside
>>>>> vma_pin_iomap(). It looks like there is an internal patch that tries
>>>>> to handle both issues, so I guess we need to also bring that patch
>>>>> upstream as a prerequisite to this?
>>>>
>>>> I have above in intel_dpt_pin(..) that "pin_flags |= PIN_MAPPABLE" when
>>>> handling stolen memory. I suspect patch you are referring to is this
>>>> same patch I wrote, here just adjusted for upstreaming. This patch was
>>>> earlier tried by Lucas and Manasi to be working with adlp and apparently
>>>> cases with virtual machine this make it possible to have tiled
>>>> framebuffers. Without this patch those special cases will get -e2big
>>>> when creating tiled fb and no stolen memory available.
>>>
>>> When the GGTT pin eventually ends up returning some vma that is not
>>> within the ggtt->mappable_end, then we will start hitting the above
>>> issues, starting with the WARN_ON. If you use PIN_HIGH here for the
>>> non-stolen case, it should highlight the issue more reliably I think.
>>>
>>
>> You mean once there's no space left in stolen there would be WARN_ON()?
>> This is case which was earlier tested by Lucas and Manasi on adlp to be
>> working correctly, this was on top of drm-tip. Also on internal testing
>> you can see platforms taking this path reliably with no errors.
>>
>> I'm not sure why use PIN_HIGH for non stolen case, my exposure to gem
>> related parts is limited hence I was following Chris's suggestion to put
>> zero flag for i915_gem_object_ggtt_pin_ww(..) when not using stolen.
>
> Asking for PIN_MAPPABLE ensures that the vma is always placed within
> the mappable part of the GGTT(i.e ggtt->mappable_end), which is
> usually only the first 256M of the GGTT. If we don't ask for
> PIN_MAPPABLE(which is what this patch is doing for the non-stolen
> case) then the vma might now be placed outside of the special mappable
> range. This mappable range has a corresponding MMIO window which lets
> us access, via some CPU address, the memory pointed at by those GGTT
> PTEs. In the case of stolen system-memory this is the only known
> reliable way to access such memory from the CPU. However if this is
> just normal system memory(which is what this patch is now doing) then
> we can just map it directly and don't need the PIN_MAPPABLE thing, but
> that is exactly the piece we are also missing in vma_pin_iomap. The
> WARN_ON(!i915_vma_is_map_and_fenceable(vma)) should catch this issue,
> but I assume it's only by coincidence that we are not hitting it with
> this patch, since the vma just so happens to be in the mappable
> part(?), but that won't always be the case. If you look at the
> internal version of vma_pin_iomap() there is some extra code to handle
> this, which I guess should be a prerequisite to this patch.
Ah, now I see what you meant. I think I found essential parts of code,
I'll look into those and see what's needed. Thanks.
/Juha-Pekka
More information about the Intel-gfx
mailing list