[Intel-gfx] [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Sep 26 09:58:49 UTC 2019
On 26/09/2019 10:14, Ramalingam C wrote:
> 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 at intel.com>
>>> Signed-off-by: Ramalingam C <ramalingam.c at 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.
Yes, and making a kernel copy. AFAICS i == uregions at the end of the
loop and args->size worth of u32 has been copied. Equivalent to
copy_from_user(uregions_copy, uregions, args->size * sizeof(u32)). Maybe
I am blind yet again..
>>
>>> +
>>> + 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.
Oh it's at the top of the patch. My bad for forgetting about it. The
region copying loop caused some head scratching and I forgot. :)
Regards,
Tvrtko
More information about the Intel-gfx
mailing list