On 2019-09-26 at 09:53:03 +0100, Tvrtko Ursulin wrote:
>
> On 26/09/2019 06:21, Ramalingam C wrote:
> > If Local memory is supported by hardware, we want framebuffer backing
> > gem objects out of local memory.
> >
> > If local memory is supported and gem object if not from local memory we
> > migrate the obj into local memory. And once framebuffer is created we
> > block the migration of the associated object out of local memory.
> >
> > This is developed on top of v3 LMEM series
> > https://patchwork.freedesktop.org/series/56683/
> >
> > cc: Matthew Auld <matthew.a...@intel.com>
> > Signed-off-by: Ramalingam C <ramalinga...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 58 ++++++++++++--------
> > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +
> > 3 files changed, 62 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1cc74844d3ea..d1921a317066 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -56,6 +56,8 @@
> > #include "display/intel_tv.h"
> > #include "display/intel_vdsc.h"
> > +#include "gem/i915_gem_object.h"
> > +
> > #include "i915_drv.h"
> > #include "i915_trace.h"
> > #include "intel_acpi.h"
> > @@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct
> > drm_i915_private *dev_priv)
> > static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> > {
> > struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > + struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > +
> > + /* removing the FB memory region restriction on obj, if any */
> > + intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;
>
> Is this right, assigning bitmask to something called obj?
Oops. wanted to assign into the obj->memory_regions. Thanks for
catching it.
>
> > drm_framebuffer_cleanup(fb);
> > intel_frontbuffer_put(intel_fb->frontbuffer);
> > @@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> > {
> > struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > struct drm_framebuffer *fb = &intel_fb->base;
> > + u32 *region_map;
> > u32 max_stride;
> > unsigned int tiling, stride;
> > int ret = -EINVAL;
> > int i;
> > + /* GEM Obj for frame buffer is expected to be in LMEM. */
> > + if (HAS_LMEM(dev_priv))
> > + if (obj->mm.region->type != INTEL_LMEM) {
> > + region_map = &intel_region_map[INTEL_MEMORY_LMEM];
> > + ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
> > + region_map,
> > + 1);
> > + if (ret) {
> > + DRM_ERROR("FB migration to LMEM Failed(%d)\n",
> > + ret);
>
> Probably should be just debug level since it is imaginably user triggerable
> and not really an error for the kernel as such.
Sure.
>
> > + return ret;
> > + }
> > + }
> > +
> > intel_fb->frontbuffer = intel_frontbuffer_get(obj);
> > if (!intel_fb->frontbuffer)
> > return -ENOMEM;
> > @@ -15666,6 +15687,10 @@ static int intel_framebuffer_init(struct
> > intel_framebuffer *intel_fb,
> > goto err;
> > }
> > + /* Blocking the migration of gem obj out of LMEM */
> > + if (HAS_LMEM(dev_priv))
> > + obj->memory_regions = REGION_LMEM;
> > +
> > return 0;
> > err:
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index e6f8426dedff..65b47054130b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -501,30 +501,11 @@ __region_id(u32 region)
> > return INTEL_MEMORY_UKNOWN;
> > }
> > -static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> > - struct drm_i915_gem_object_param *args,
> > - struct drm_file *file,
> > - struct drm_i915_gem_object *obj)
> > +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
> > + struct drm_i915_gem_object *obj,
> > + u32 *uregions, u32 region_count)
> > {
> > struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
> > - u32 __user *uregions = u64_to_user_ptr(args->data);
> > - u32 uregions_copy[INTEL_MEMORY_UKNOWN];
> > - int i, ret;
> > -
> > - if (args->size > INTEL_MEMORY_UKNOWN)
> > - return -EINVAL;
> > -
> > - memset(uregions_copy, 0, sizeof(uregions_copy));
> > - for (i = 0; i < args->size; i++) {
> > - u32 region;
> > -
> > - ret = get_user(region, uregions);
> > - if (ret)
> > - return ret;
> > -
> > - uregions_copy[i] = region;
> > - ++uregions;
> > - }
> > mutex_lock(&dev_priv->drm.struct_mutex);
> > ret = i915_gem_object_prepare_move(obj);
> > @@ -533,8 +514,8 @@ static int i915_gem_object_region_select(struct
> > drm_i915_private *dev_priv,
> > goto err;
> > }
> > - for (i = 0; i < args->size; i++) {
> > - u32 region = uregions_copy[i];
> > + for (i = 0; i < region_count; i++) {
> > + u32 region = uregions[i];
> > enum intel_region_id id = __region_id(region);
> > if (!(obj->memory_region & region)) {
> > @@ -576,6 +557,35 @@ static int i915_gem_object_region_select(struct
> > drm_i915_private *dev_priv,
> > return ret;
> > }
> > +static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> > + struct drm_i915_gem_object_param *args,
> > + struct drm_file *file,
> > + struct drm_i915_gem_object *obj)
> > +{
> > + struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
> > + u32 __user *uregions = u64_to_user_ptr(args->data);
> > + u32 uregions_copy[INTEL_MEMORY_UKNOWN];
> > + int i, ret;
> > +
> > + if (args->size > INTEL_MEMORY_UKNOWN)
> > + return -EINVAL;
> > +
> > + memset(uregions_copy, 0, sizeof(uregions_copy));
> > + for (i = 0; i < args->size; i++) {
> > + u32 region;
> > +
> > + ret = get_user(region, uregions);
> > + if (ret)
> > + return ret;
> > +
> > + uregions_copy[i] = region;
> > + ++uregions;
> > + }
>
> I do understand this is old code (not yours) but I fail to penetrate why is
> this a loop and not copy_from_user?
What i understood is, list of possible regions_maps are passed here. and
they are reading it one after another.
>
> > +
> > + return i915_gem_object_mem_region_migrate(dev_priv, obj, uregions_copy,
> > + args->size);
> > +}
> > +
> > int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file)
> > {
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > index a7c073aeb777..d09a9c741b41 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > @@ -47,6 +47,9 @@ int i915_gem_object_migrate(struct drm_i915_gem_object
> > *obj,
> > enum intel_region_id id);
> > void i915_gem_flush_free_objects(struct drm_i915_private *i915);
> > +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
> > + struct drm_i915_gem_object *obj,
> > + u32 *uregions, u32 region_count);
> > void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj);
> >
>
> General approach looks feasible to me, but I'll leave guys directly involved
> with local memory to call it.
>
> Another my comment/question would be - do we have a hook when objects are
> released/disassociated from the framebuffer and should we restore the
> allowed memory region mask to default at that point?
at present I am doing this at intel_user_framebuffer_destroy. Not sure
if we have any better place for this.
-Ram
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
