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@xxxxxxxxxxxx> > --- <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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
