Re: [Intel-gfx] [PATCH 1/3] drm: Add new DRM_IOCTL_MODE_GETPLANE2

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 11, 2017 at 04:51:16PM -0800, Ben Widawsky wrote:
> Originally based off of a patch by Kristian.
> 
> This new ioctl extends DRM_IOCTL_MODE_GETPLANE, by returning information
> about the modifiers that will work with each format.
> 
> It's modified from Kristian's patch in that the modifiers and formats
> are setup by the driver, and then a callback is used to create the
> format list. The LOC was enough difference that I don't think it made
> sense to leave his authorship, but the new UABI was primarily his idea.
> 
> Additionally, I hit a couple of drivers which Kristian missed updating.
> 
> It also contains a change requested by Daniel to make the modifiers
> array a sentinel based structure instead of a sized one. Upon discussion
> on IRC, it was determined that having an invalid modifier might make
> sense in general as well.
> 
> References: https://patchwork.kernel.org/patch/9482393/
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
<snip>
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index 2b33825f2f93..9cb1eede0b4d 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -124,6 +124,7 @@ static struct drm_plane *create_primary_plane(struct drm_device *dev)
>  				       &drm_primary_helper_funcs,
>  				       safe_modeset_formats,
>  				       ARRAY_SIZE(safe_modeset_formats),
> +				       NULL,
>  				       DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret) {
>  		kfree(primary);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 7b7275f0c2df..2d4fad5db8ed 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -70,6 +70,7 @@ static unsigned int drm_num_planes(struct drm_device *dev)
>   * @funcs: callbacks for the new plane
>   * @formats: array of supported formats (DRM_FORMAT\_\*)
>   * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by INVALID
>   * @type: type of plane (overlay, primary, cursor)
>   * @name: printf style format string for the plane name, or NULL for default name
>   *
> @@ -82,10 +83,12 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     uint32_t possible_crtcs,
>  			     const struct drm_plane_funcs *funcs,
>  			     const uint32_t *formats, unsigned int format_count,
> +			     const uint64_t *format_modifiers,
>  			     enum drm_plane_type type,
>  			     const char *name, ...)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> +	unsigned int format_modifier_count = 0;
>  	int ret;
>  
>  	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> @@ -105,6 +108,28 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		return -ENOMEM;
>  	}
>  
> +	if (format_modifiers) {
> +		const uint64_t *temp_modifiers = format_modifiers;
> +		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> +			format_modifier_count++;
> +	}
> +
> +	if (format_modifier_count)
> +		DRM_DEBUG_KMS("%d format modifiers added to list\n",
> +			      format_modifier_count);

nit: Not sure this is printk worthy.

> +
> +	plane->modifier_count = format_modifier_count;
> +	plane->modifiers = kmalloc_array(format_modifier_count,
> +					 sizeof(format_modifiers[0]),
> +					 GFP_KERNEL);
> +
> +	if (format_modifier_count && !plane->modifiers) {
> +		DRM_DEBUG_KMS("out of memory when allocating plane\n");
> +		kfree(plane->format_types);
> +		drm_mode_object_unregister(dev, &plane->base);
> +		return -ENOMEM;
> +	}
> +
>  	if (name) {
>  		va_list ap;
>  
> @@ -117,12 +142,15 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	}
>  	if (!plane->name) {
>  		kfree(plane->format_types);
> +		kfree(plane->modifiers);
>  		drm_mode_object_unregister(dev, &plane->base);
>  		return -ENOMEM;
>  	}
>  
>  	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
>  	plane->format_count = format_count;
> +	memcpy(plane->modifiers, format_modifiers,
> +	       format_modifier_count * sizeof(format_modifiers[0]));

Looks all right since we do the same for formats anyway. But it did
occur to me (twice at least) that a kmemdup_array() might a nice thing
to have for things like this. But that's a separate topic.

>  	plane->possible_crtcs = possible_crtcs;
>  	plane->type = type;
>  
> @@ -205,7 +233,8 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	type = is_primary ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
>  	return drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
> -					formats, format_count, type, NULL);
> +					formats, format_count,
> +					NULL, type, NULL);
>  }
>  EXPORT_SYMBOL(drm_plane_init);
>  
> @@ -224,6 +253,7 @@ void drm_plane_cleanup(struct drm_plane *plane)
>  	drm_modeset_lock_fini(&plane->mutex);
>  
>  	kfree(plane->format_types);
> +	kfree(plane->modifiers);
>  	drm_mode_object_unregister(dev, &plane->base);
>  
>  	BUG_ON(list_empty(&plane->head));
> @@ -380,12 +410,15 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>  int drm_mode_getplane(struct drm_device *dev, void *data,
>  		      struct drm_file *file_priv)
>  {
> -	struct drm_mode_get_plane *plane_resp = data;
> +	struct drm_mode_get_plane2 *plane_resp = data;
>  	struct drm_plane *plane;
>  	uint32_t __user *format_ptr;
> +	struct drm_format_modifier __user *modifier_ptr;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> +	if (plane_resp->flags)
> +		return -EINVAL;
>  
>  	plane = drm_plane_find(dev, plane_resp->plane_id);
>  	if (!plane)
> @@ -426,6 +459,36 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	}
>  	plane_resp->count_format_types = plane->format_count;
>  
> +	if (plane->modifier_count &&
> +	    plane_resp->count_format_modifiers >= plane->modifier_count) {
> +		struct drm_format_modifier mod;
> +		int i;
> +
> +		modifier_ptr = (struct drm_format_modifier __user *)
> +			(unsigned long)plane_resp->format_modifier_ptr;

Didn't we have some something_user_ptr() thing? Hmm, I guess it's in i915 only.

> +
> +		/* Build the mask for each modifier */
> +		for (i = 0; i < plane->modifier_count; i++) {
> +			int j;
> +			mod.modifier = plane->modifiers[i];
> +			for (j = 0; j < plane->format_count; j++) {

If we don't want to try and deal with more than 64 formats, I think we
need to make drm_universal_plane_init() WARN+bail if the driver
passes in more than that.

> +				if (plane->funcs->format_mod_supported &&
> +				    plane->funcs->format_mod_supported(plane,
> +								       plane->format_types[j],
> +								       plane->modifiers[i])) {
> +					mod.formats |= 1 << j;
> +				}
> +			}
> +
> +			if (copy_to_user(modifier_ptr, &mod, sizeof(mod)))
> +				return -EFAULT;
> +
> +			modifier_ptr++;
> +		}
> +	}
> +
> +	plane_resp->count_format_modifiers = plane->modifier_count;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 35c5d99296b9..0406e71b38e8 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -193,6 +193,7 @@ EXPORT_SYMBOL(drm_simple_display_pipe_attach_bridge);
>   * @funcs: callbacks for the display pipe (optional)
>   * @formats: array of supported formats (DRM_FORMAT\_\*)
>   * @format_count: number of elements in @formats
> + * @modifiers: array of formats modifiers

@format_modifiers

>   * @connector: connector to attach and register (optional)
>   *
>   * Sets up a display pipeline which consist of a really simple
> @@ -213,6 +214,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  			struct drm_simple_display_pipe *pipe,
>  			const struct drm_simple_display_pipe_funcs *funcs,
>  			const uint32_t *formats, unsigned int format_count,
> +			const uint64_t *format_modifiers,
>  			struct drm_connector *connector)
>  {
>  	struct drm_encoder *encoder = &pipe->encoder;
> @@ -227,6 +229,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
>  	ret = drm_universal_plane_init(dev, plane, 0,
>  				       &drm_simple_kms_plane_funcs,
>  				       formats, format_count,
> +				       format_modifiers,
>  				       DRM_PLANE_TYPE_PRIMARY, NULL);
>  	if (ret)
>  		return ret;
<snip>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index ce7efe2e8a5e..cea3de3aa301 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -209,6 +209,33 @@ struct drm_mode_get_plane {
>  	__u64 format_type_ptr;
>  };
>  
> +struct drm_format_modifier {
> +	/* Bitmask of formats in get_plane format list this info
> +	 * applies to. */
> +	uint64_t formats;
> +
> +	/* This modifier can be used with the format for this plane. */
> +	uint64_t modifier;
> +};
> +
> +struct drm_mode_get_plane2 {
> +	__u32 plane_id;
> +
> +	__u32 crtc_id;
> +	__u32 fb_id;
> +
> +	__u32 possible_crtcs;
> +	__u32 gamma_size;
> +
> +	__u32 count_format_types;
> +	__u64 format_type_ptr;
> +
> +	/* New in v2 */
> +	__u32 count_format_modifiers;
> +	__u32 flags;

ocd induced idea: Maybe put flags before the count?

> +	__u64 format_modifier_ptr;
> +};
> +
>  struct drm_mode_get_plane_res {
>  	__u64 plane_id_ptr;
>  	__u32 count_planes;

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel