| Message ID | 20220506131109.20942-1-juhapekka.heikkila@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/display: Add smem fallback allocation for dpt | expand |
On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote: > > Add fallback smem allocation for dpt if stolen memory allocation failed. > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@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? > > 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@intel.com> > + dpt_obj = i915_gem_object_create_internal(i915, size); > + } > if (IS_ERR(dpt_obj)) > return ERR_CAST(dpt_obj); > > -- > 2.25.1 >
Matthew Auld kirjoitti 11.5.2022 klo 13.41: > On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila > <juhapekka.heikkila@gmail.com> wrote: >> >> Add fallback smem allocation for dpt if stolen memory allocation failed. >> >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@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@intel.com> > >> + dpt_obj = i915_gem_object_create_internal(i915, size); >> + } >> if (IS_ERR(dpt_obj)) >> return ERR_CAST(dpt_obj); >> >> -- >> 2.25.1 >>
On Fri, 20 May 2022 at 12:23, Juha-Pekka Heikkilä <juhapekka.heikkila@gmail.com> wrote: > > > > Matthew Auld kirjoitti 11.5.2022 klo 13.41: > > On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila > > <juhapekka.heikkila@gmail.com> wrote: > >> > >> Add fallback smem allocation for dpt if stolen memory allocation failed. > >> > >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@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. What was the WARN_ON? Got a paste?
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" 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_lmem(dpt->obj)) + pin_flags |= PIN_MAPPABLE; 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"); + dpt_obj = i915_gem_object_create_internal(i915, size); + } if (IS_ERR(dpt_obj)) return ERR_CAST(dpt_obj);
Add fallback smem allocation for dpt if stolen memory allocation failed. Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> --- drivers/gpu/drm/i915/display/intel_dpt.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)