Re: [PATCH 2/2] drm/vc4: Add get/set tiling ioctls.

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1: Type: text/plain, Size: 5568 bytes --]

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> Hi Eric,
>
> On Wed,  7 Jun 2017 17:13:36 -0700
> Eric Anholt <eric@anholt.net> wrote:
>
>> This allows mesa to set the tiling format for a BO and have that
>> tiling format be respected by mesa on the other side of an
>> import/export (and by vc4 scanout in the kernel), without defining a
>> protocol to pass the tiling through userspace.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>> 
>> igt tests (which caught two edge cases) submitted to intel-gfx.
>> 
>> Mesa code: https://github.com/anholt/mesa/commits/drm-vc4-tiling
>> 
>>  drivers/gpu/drm/vc4/vc4_bo.c  | 83 +++++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/vc4/vc4_drv.c |  2 ++
>>  drivers/gpu/drm/vc4/vc4_drv.h |  6 ++++
>>  drivers/gpu/drm/vc4/vc4_kms.c | 41 ++++++++++++++++++++-
>>  include/uapi/drm/vc4_drm.h    | 16 +++++++++
>>  5 files changed, 147 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>> index 80b2f9e55c5c..21649109fd4f 100644
>> --- a/drivers/gpu/drm/vc4/vc4_bo.c
>> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>> @@ -347,6 +347,7 @@ void vc4_free_object(struct drm_gem_object *gem_bo)
>>  		bo->validated_shader = NULL;
>>  	}
>>  
>> +	bo->t_format = false;
>>  	bo->free_time = jiffies;
>>  	list_add(&bo->size_head, cache_list);
>>  	list_add(&bo->unref_head, &vc4->bo_cache.time_list);
>> @@ -572,6 +573,88 @@ vc4_create_shader_bo_ioctl(struct drm_device *dev, void *data,
>>  	return ret;
>>  }
>>  
>> +/**
>> + * vc4_set_tiling_ioctl() - Sets the tiling modifier for a BO.
>> + * @dev: DRM device
>> + * @data: ioctl argument
>> + * @file_priv: DRM file for this fd
>> + *
>> + * The tiling state of the BO decides the default modifier of an fb if
>> + * no specific modifier was set by userspace, and the return value of
>> + * vc4_get_tiling_ioctl() (so that userspace can treat a BO it
>> + * received from dmabuf as the same tiling format as the producer
>> + * used).
>> + */
>> +int vc4_set_tiling_ioctl(struct drm_device *dev, void *data,
>> +			 struct drm_file *file_priv)
>> +{
>> +	struct drm_vc4_set_tiling *args = data;
>> +	struct drm_gem_object *gem_obj;
>> +	struct vc4_bo *bo;
>> +	bool t_format;
>> +
>> +	if (args->flags != 0)
>> +		return -EINVAL;
>> +
>> +	switch (args->modifier) {
>> +	case DRM_FORMAT_MOD_NONE:
>> +		t_format = false;
>> +		break;
>> +	case DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED:
>> +		t_format = true;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>> +	if (!gem_obj) {
>> +		DRM_ERROR("Failed to look up GEM BO %d\n", args->handle);
>> +		return -ENOENT;
>> +	}
>> +	bo = to_vc4_bo(gem_obj);
>> +	bo->t_format = t_format;
>> +
>> +	drm_gem_object_unreference_unlocked(gem_obj);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * vc4_get_tiling_ioctl() - Gets the tiling modifier for a BO.
>> + * @dev: DRM device
>> + * @data: ioctl argument
>> + * @file_priv: DRM file for this fd
>> + *
>> + * Returns the tiling modifier for a BO as set by vc4_set_tiling_ioctl().
>> + */
>> +int vc4_get_tiling_ioctl(struct drm_device *dev, void *data,
>> +			 struct drm_file *file_priv)
>> +{
>> +	struct drm_vc4_get_tiling *args = data;
>> +	struct drm_gem_object *gem_obj;
>> +	struct vc4_bo *bo;
>> +
>> +	if (args->flags != 0 || args->modifier != 0)
>> +		return -EINVAL;
>> +
>> +	gem_obj = drm_gem_object_lookup(file_priv, args->handle);
>> +	if (!gem_obj) {
>> +		DRM_ERROR("Failed to look up GEM BO %d\n", args->handle);
>> +		return -ENOENT;
>> +	}
>> +	bo = to_vc4_bo(gem_obj);
>> +
>> +	if (bo->t_format)
>> +		args->modifier = DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
>> +	else
>> +		args->modifier = DRM_FORMAT_MOD_NONE;
>> +
>> +	drm_gem_object_unreference_unlocked(gem_obj);
>> +
>> +	return 0;
>> +}
>> +
>>  void vc4_bo_cache_init(struct drm_device *dev)
>>  {
>>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
>> index 136bb4213dc0..c6b487c3d2b7 100644
>> --- a/drivers/gpu/drm/vc4/vc4_drv.c
>> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
>> @@ -138,6 +138,8 @@ static const struct drm_ioctl_desc vc4_drm_ioctls[] = {
>>  	DRM_IOCTL_DEF_DRV(VC4_GET_HANG_STATE, vc4_get_hang_state_ioctl,
>>  			  DRM_ROOT_ONLY),
>>  	DRM_IOCTL_DEF_DRV(VC4_GET_PARAM, vc4_get_param_ioctl, DRM_RENDER_ALLOW),
>> +	DRM_IOCTL_DEF_DRV(VC4_SET_TILING, vc4_set_tiling_ioctl, DRM_RENDER_ALLOW),
>> +	DRM_IOCTL_DEF_DRV(VC4_GET_TILING, vc4_get_tiling_ioctl, DRM_RENDER_ALLOW),
>>  };
>>  
>>  static struct drm_driver vc4_drm_driver = {
>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>> index a5bf2e5e0b57..df22698d62ee 100644
>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>> @@ -148,6 +148,8 @@ struct vc4_bo {
>>  	 */
>>  	uint64_t write_seqno;
>>  
>> +	bool t_format;
>> +
>
> Will we need the DRM_VC4_SET/GET_TILING ioctls when importing a BO
> that is using H264 tile mode? If this is the case, we should probably
> store the modifier directly.

I'm not sure.  Whoever is getting buffers from the ISP is going to be
doing the prime import to vc4 for displaying it on a plane, so it seems
about equal complexity ot me to do it either way.  If we're using some
existing dma-buf based media stack, it might support plane modifiers
already, though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]