Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@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?

>   
>   	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.

> +				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?

> +
> +	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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx