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.auld@xxxxxxxxx> > > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
