Date: Tue, 13 Jun 2017 09:46:45 +0200
From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Eric Anholt <eric@anholt.net>
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] drm/vc4: Add get/set tiling ioctls.
Message-ID: <20170613094645.625031a5@bbrezillon>
In-Reply-To: <20170608001336.12842-2-eric@anholt.net>
References: <20170608001336.12842-1-eric@anholt.net>
        <20170608001336.12842-2-eric@anholt.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9136
Lines: 264

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.

>  	/* List entry for the BO's position in either
>  	 * vc4_exec_info->unref_list or vc4_dev->bo_cache.time_list
>  	 */
> @@ -470,6 +472,10 @@ int vc4_create_shader_bo_ioctl(struct drm_device *dev, void *data,
>  			       struct drm_file *file_priv);
>  int vc4_mmap_bo_ioctl(struct drm_device *dev, void *data,
>  		      struct drm_file *file_priv);
> +int vc4_set_tiling_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_priv);
> +int vc4_get_tiling_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file_priv);
>  int vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
>  			     struct drm_file *file_priv);
>  int vc4_mmap(struct file *filp, struct vm_area_struct *vma);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 928d191ef90f..202f7ebf5a7b 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -202,11 +202,50 @@ static int vc4_atomic_commit(struct drm_device *dev,
>  	return 0;
>  }
>  
> +static struct drm_framebuffer *vc4_fb_create(struct drm_device *dev,
> +					     struct drm_file *file_priv,
> +					     const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	struct drm_mode_fb_cmd2 mode_cmd_local;
> +
> +	/* If the user didn't specify a modifier, use the
> +	 * vc4_set_tiling_ioctl() state for the BO.
> +	 */
> +	if (!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
> +		struct drm_gem_object *gem_obj;
> +		struct vc4_bo *bo;
> +
> +		gem_obj = drm_gem_object_lookup(file_priv,
> +						mode_cmd->handles[0]);
> +		if (!gem_obj) {
> +			DRM_ERROR("Failed to look up GEM BO %d\n",
> +				  mode_cmd->handles[0]);
> +			return ERR_PTR(-ENOENT);
> +		}
> +		bo = to_vc4_bo(gem_obj);
> +
> +		mode_cmd_local = *mode_cmd;
> +
> +		if (bo->t_format) {
> +			mode_cmd_local.modifier[0] =
> +				DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
> +		} else {
> +			mode_cmd_local.modifier[0] = DRM_FORMAT_MOD_NONE;
> +		}
> +
> +		drm_gem_object_unreference_unlocked(gem_obj);
> +
> +		mode_cmd = &mode_cmd_local;
> +	}
> +
> +	return drm_fb_cma_create(dev, file_priv, mode_cmd);
> +}
> +
>  static const struct drm_mode_config_funcs vc4_mode_funcs = {
>  	.output_poll_changed = vc4_output_poll_changed,
>  	.atomic_check = drm_atomic_helper_check,
>  	.atomic_commit = vc4_atomic_commit,
> -	.fb_create = drm_fb_cma_create,
> +	.fb_create = vc4_fb_create,
>  };
>  
>  int vc4_kms_load(struct drm_device *dev)
> diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
> index f07a09016726..6ac4c5c014cb 100644
> --- a/include/uapi/drm/vc4_drm.h
> +++ b/include/uapi/drm/vc4_drm.h
> @@ -38,6 +38,8 @@ extern "C" {
>  #define DRM_VC4_CREATE_SHADER_BO                  0x05
>  #define DRM_VC4_GET_HANG_STATE                    0x06
>  #define DRM_VC4_GET_PARAM                         0x07
> +#define DRM_VC4_SET_TILING                        0x08
> +#define DRM_VC4_GET_TILING                        0x09
>  
>  #define DRM_IOCTL_VC4_SUBMIT_CL           DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_SUBMIT_CL, struct drm_vc4_submit_cl)
>  #define DRM_IOCTL_VC4_WAIT_SEQNO          DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_WAIT_SEQNO, struct drm_vc4_wait_seqno)
> @@ -47,6 +49,8 @@ extern "C" {
>  #define DRM_IOCTL_VC4_CREATE_SHADER_BO    DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_CREATE_SHADER_BO, struct drm_vc4_create_shader_bo)
>  #define DRM_IOCTL_VC4_GET_HANG_STATE      DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_GET_HANG_STATE, struct drm_vc4_get_hang_state)
>  #define DRM_IOCTL_VC4_GET_PARAM           DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_GET_PARAM, struct drm_vc4_get_param)
> +#define DRM_IOCTL_VC4_SET_TILING          DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_SET_TILING, struct drm_vc4_set_tiling)
> +#define DRM_IOCTL_VC4_GET_TILING          DRM_IOWR(DRM_COMMAND_BASE + DRM_VC4_GET_TILING, struct drm_vc4_get_tiling)
>  
>  struct drm_vc4_submit_rcl_surface {
>  	__u32 hindex; /* Handle index, or ~0 if not present. */
> @@ -295,6 +299,18 @@ struct drm_vc4_get_param {
>  	__u64 value;
>  };
>  
> +struct drm_vc4_get_tiling {
> +	__u32 handle;
> +	__u32 flags;
> +	__u64 modifier;
> +};
> +
> +struct drm_vc4_set_tiling {
> +	__u32 handle;
> +	__u32 flags;
> +	__u64 modifier;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif