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