[Intel-gfx] [PATCH 1/2] drm/i915/display: Add smem fallback allocation for dpt

[Juha-Pekka Heikkilä]

Matthew Auld kirjoitti 11.5.2022 klo 13.41:
> On Fri, 6 May 2022 at 14:11, 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 | 16 ++++++++++++----
>>   1 file changed, 12 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..10008699656e 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: Keep these ordered.
> 
>>
>>   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;
> 
> Nit: Christmas tree-ish. Move this above the err.
> 
>> +
>> +       if (!i915_gem_object_is_lmem(dpt->obj))
>> +               pin_flags |= PIN_MAPPABLE;
> 
> If we do this then we don't need the second patch ;)
> 
> I guess the second patch was meant to make this is_stolen? Maybe just
> move the second patch to be the first in the series?
> 

Hi Matthew, thanks for the comments. I think I'm still missing some 
essential part. Without marking PIN_MAPPABLE when !lmem I was hitting 
WARN_ON() in gem code when doing this pinning. Weird thing with it was I 
got everything working with y-tile but x-tile was 100% fail. I'll need 
to repro those results and see why I got that. There was recently fixed 
regression on igt side which may have affected my results but I'll 
probably anyway need to have another round for these patches including 
fixes to those parts you pointed out.

/Juha-Pekka

>>
>>          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,13 @@ 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, "Allocating dpt from smem\n");
> 
> Hopefully this is not too noisy?
> 
> With the s/is_lmem/is_stolen/, or however you want to deal with that,
> Reviewed-by: Matthew Auld <matthew.auld at intel.com>
> 
>> +               dpt_obj = i915_gem_object_create_internal(i915, size);
>> +       }
>>          if (IS_ERR(dpt_obj))
>>                  return ERR_CAST(dpt_obj);
>>
>> --
>> 2.25.1
>>