| Message ID | 1393539283-5901-2-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 27, 2014 at 02:14:40PM -0800, Matt Roper wrote: > Allow drivers to provide a drm_plane structure corresponding to a CRTC's > primary plane. These planes will be included in the plane list for any > clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Two small notes here and comments inside: - In term of new API enabling and to make the tree bisectable, one needs to 1/ do the preparation work 2/ enable the API. In this case, I would split up the patch to make the ioctl bits independent and the last patch of the series. - I don't see a point in introducing the complexity to enable sprite and cursor planes independently from each other. So before enabling the new setcap() ioctl, could we also expose the cursor planes and call the capability "universal planes" or similar?
On Mon, Mar 03, 2014 at 03:47:43PM +0000, Damien Lespiau wrote: > On Thu, Feb 27, 2014 at 02:14:40PM -0800, Matt Roper wrote: > > Allow drivers to provide a drm_plane structure corresponding to a CRTC's > > primary plane. These planes will be included in the plane list for any > > clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Two small notes here and comments inside: > > - In term of new API enabling and to make the tree bisectable, one needs > to 1/ do the preparation work 2/ enable the API. In this case, I would > split up the patch to make the ioctl bits independent and the last > patch of the series. > > - I don't see a point in introducing the complexity to enable sprite and > cursor planes independently from each other. So before enabling the > new setcap() ioctl, could we also expose the cursor planes and call > the capability "universal planes" or similar? I'm not sure I follow what you're saying on this second point. Sprites (or overlays) are already exposed to userspace, so existing clients expect anything they receive via drmModeGetPlaneResources() to behave in the traditional manner. If we start throwing cursor planes into that list without hiding them behind a capability bit, existing userspace try to blindly use the cursors as full overlays without realizing that they may carry additional restrictions on size and such, which will lead to userspace breakage. In cases where userspace has set a capability bit, I agree that sprites/overlays/cursors could all pretty much be treated the same as long as there are additional plane properties to let userspace understand the exact restrictions of each plane. Is that what you were referring to here? I agree with your other comments; I'll work on a second revision that incorporates the suggestions from you and Rob. Thanks for the review. Matt > > -- > Damien > > > --- > > drivers/gpu/drm/drm_crtc.c | 168 ++++++++++++++++++++++++++++++++++++++++++-- > > drivers/gpu/drm/drm_ioctl.c | 5 ++ > > include/drm/drmP.h | 2 + > > include/drm/drm_crtc.h | 11 +++ > > include/uapi/drm/drm.h | 8 +++ > > 5 files changed, 189 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 35ea15d..21c6d4b 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status) > > } > > EXPORT_SYMBOL(drm_get_connector_status_name); > > > > +/* > > + * Default set of pixel formats supported by primary planes. This matches the > > + * set of formats accepted by the traditional modesetting interfaces; drivers > > + * need only provide their own format list if it differs from the default. > > + */ > > +static const uint32_t default_primary_plane_formats[] = { > > + DRM_FORMAT_C8, > > + DRM_FORMAT_RGB332, > > + DRM_FORMAT_BGR233, > > + DRM_FORMAT_XRGB4444, > > + DRM_FORMAT_XBGR4444, > > + DRM_FORMAT_RGBX4444, > > + DRM_FORMAT_BGRX4444, > > + DRM_FORMAT_ARGB4444, > > + DRM_FORMAT_ABGR4444, > > + DRM_FORMAT_RGBA4444, > > + DRM_FORMAT_BGRA4444, > > + DRM_FORMAT_XRGB1555, > > + DRM_FORMAT_XBGR1555, > > + DRM_FORMAT_RGBX5551, > > + DRM_FORMAT_BGRX5551, > > + DRM_FORMAT_ARGB1555, > > + DRM_FORMAT_ABGR1555, > > + DRM_FORMAT_RGBA5551, > > + DRM_FORMAT_BGRA5551, > > + DRM_FORMAT_RGB565, > > + DRM_FORMAT_BGR565, > > + DRM_FORMAT_RGB888, > > + DRM_FORMAT_BGR888, > > + DRM_FORMAT_XRGB8888, > > + DRM_FORMAT_XBGR8888, > > + DRM_FORMAT_RGBX8888, > > + DRM_FORMAT_BGRX8888, > > + DRM_FORMAT_ARGB8888, > > + DRM_FORMAT_ABGR8888, > > + DRM_FORMAT_RGBA8888, > > + DRM_FORMAT_BGRA8888, > > + DRM_FORMAT_XRGB2101010, > > + DRM_FORMAT_XBGR2101010, > > + DRM_FORMAT_RGBX1010102, > > + DRM_FORMAT_BGRX1010102, > > + DRM_FORMAT_ARGB2101010, > > + DRM_FORMAT_ABGR2101010, > > + DRM_FORMAT_RGBA1010102, > > + DRM_FORMAT_BGRA1010102, > > + DRM_FORMAT_YUYV, > > + DRM_FORMAT_YVYU, > > + DRM_FORMAT_UYVY, > > + DRM_FORMAT_VYUY, > > + DRM_FORMAT_AYUV, > > + DRM_FORMAT_NV12, > > + DRM_FORMAT_NV21, > > + DRM_FORMAT_NV16, > > + DRM_FORMAT_NV61, > > + DRM_FORMAT_NV24, > > + DRM_FORMAT_NV42, > > + DRM_FORMAT_YUV410, > > + DRM_FORMAT_YVU410, > > + DRM_FORMAT_YUV411, > > + DRM_FORMAT_YVU411, > > + DRM_FORMAT_YUV420, > > + DRM_FORMAT_YVU420, > > + DRM_FORMAT_YUV422, > > + DRM_FORMAT_YVU422, > > + DRM_FORMAT_YUV444, > > + DRM_FORMAT_YVU444, > > +}; > > + > > /** > > * drm_get_subpixel_order_name - return a string for a given subpixel enum > > * @order: enum of subpixel_order > > @@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > > EXPORT_SYMBOL(drm_encoder_cleanup); > > > > /** > > - * drm_plane_init - Initialise a new plane object > > + * drm_plane_init - Initialise a new sprite plane object > > * @dev: DRM device > > * @plane: plane object to init > > * @possible_crtcs: bitmask of possible CRTCs > > @@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup); > > * @format_count: number of elements in @formats > > * @priv: plane is private (hidden from userspace)? > > * > > - * Inits a new object created as base part of a driver plane object. > > + * Inits a new object created as base part of a driver plane object. This > > + * function should only be called for traditional sprite/overlay planes. > > + * Primary planes should be initialized via @drm_plane_set_primary. > > * > > * RETURNS: > > * Zero on success, error code on failure. > > @@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > > EXPORT_SYMBOL(drm_plane_init); > > > > /** > > + * drm_plane_set_primary - Supply a primary plane for a CRTC > > + * @dev DRM device > > + * @plane: plane object representing the primary plane > > + * @crtc: CRTC this plane is associated with > > + * @funcs: callbacks for the new plane > > + * @formats: array of supported formats (%DRM_FORMAT_*). If NULL, the > > + * default list of formats traditionally supported by modesetting > > + * API's will be used and @format_count will be ignored. > > + * @format_count: number of elements in @formats > > + * > > + * Provides a drm_plane representing a CRTC's primary plane. This plane will > > + * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES > > + * capability bit. > > + * > > + * RETURNS: > > + * Zero on success, error code on failure. > > + */ > > +int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, > > + struct drm_crtc *crtc, > > + const struct drm_plane_funcs *funcs, > > + const uint32_t *formats, uint32_t format_count) > > The "priv" parameter of drm_plane_init() seems to be used by drivers to > initialize those planes attached to the CRTC object (needs double > checking). Could we refactor drm_plane_init() so drm_plane_set_primary() > calls drm_plane_init() with priv=true (argument that could then renamed > to primary). > > > +{ > > + int ret; > > + > > + drm_modeset_lock_all(dev); > > + > > + ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE); > > + if (ret) > > + goto out; > > + > > + if (formats == NULL) { > > + formats = default_primary_plane_formats; > > + format_count = ARRAY_SIZE(default_primary_plane_formats); > > + } > > We're defining a new interface here, I don't think there's any value in > providing a default set of format that is unlikely to match any > hardware. I'd just enforce that drivers need to provide the list of > acceptable formats. > > > + > > + plane->base.properties = &plane->properties; > > + plane->dev = dev; > > + plane->funcs = funcs; > > + plane->format_types = kmalloc(sizeof(uint32_t) * format_count, > > + GFP_KERNEL); > > + if (!plane->format_types) { > > + DRM_DEBUG_KMS("out of memory when setting up primary plane\n"); > > + drm_mode_object_put(dev, &plane->base); > > + ret = -ENOMEM; > > + goto out; > > + } > > + > > + memcpy(plane->format_types, formats, format_count * sizeof(uint32_t)); > > + plane->format_count = format_count; > > + plane->possible_crtcs = drm_crtc_mask(crtc); > > + > > + /* > > + * Primary planes are not added to the general plane list, only linked > > + * to their CRTC. They will only be advertised to userspace clients > > + * that indicate support. > > + */ > > + crtc->primary_plane = plane; > > + dev->mode_config.num_primary_plane++; > > + INIT_LIST_HEAD(&plane->head); > > + > > + out: > > + drm_modeset_unlock_all(dev); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(drm_plane_set_primary); > > + > > +/** > > * drm_plane_cleanup - Clean up the core plane usage > > * @plane: plane to cleanup > > * > > @@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, > > struct drm_mode_get_plane_res *plane_resp = data; > > struct drm_mode_config *config; > > struct drm_plane *plane; > > + struct drm_crtc *crtc; > > uint32_t __user *plane_ptr; > > int copied = 0, ret = 0; > > + unsigned num_planes; > > > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > > return -EINVAL; > > @@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, > > drm_modeset_lock_all(dev); > > config = &dev->mode_config; > > > > + num_planes = config->num_plane; > > + if (file_priv->expose_primary_planes) > > + num_planes += config->num_primary_plane; > > + > > /* > > * This ioctl is called twice, once to determine how much space is > > * needed, and the 2nd time to fill it. > > */ > > - if (config->num_plane && > > - (plane_resp->count_planes >= config->num_plane)) { > > + if (num_planes && > > + (plane_resp->count_planes >= num_planes)) { > > plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr; > > > > + if (file_priv->expose_primary_planes) { > > + list_for_each_entry(crtc, &config->crtc_list, head) { > > + if (!crtc->primary_plane) > > + continue; > > + > > + if (put_user(crtc->primary_plane->base.id, > > + plane_ptr + copied)) { > > + ret = -EFAULT; > > + goto out; > > + } > > + copied++; > > + } > > + } > > + > > list_for_each_entry(plane, &config->plane_list, head) { > > if (put_user(plane->base.id, plane_ptr + copied)) { > > ret = -EFAULT; > > @@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, > > copied++; > > } > > } > > - plane_resp->count_planes = config->num_plane; > > + plane_resp->count_planes = num_planes; > > > > out: > > drm_modeset_unlock_all(dev); > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > index dffc836..03579d6 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) > > return -EINVAL; > > file_priv->stereo_allowed = req->value; > > break; > > + case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES: > > + if (req->value > 1) > > + return -EINVAL; > > + file_priv->expose_primary_planes = req->value; > > + break; > > default: > > return -EINVAL; > > } > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > > index 04a7f31..997fa29 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -437,6 +437,8 @@ struct drm_file { > > unsigned is_master :1; /* this file private is a master for a minor */ > > /* true when the client has asked us to expose stereo 3D mode flags */ > > unsigned stereo_allowed :1; > > + /* true if client understands CRTC primary planes in the plane list */ > > + unsigned expose_primary_planes:1; > > > > struct pid *pid; > > kuid_t uid; > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index ce9ee60..33a955b 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -429,6 +429,11 @@ struct drm_crtc { > > * by drm_mode_set_config_internal to implement correct refcounting. */ > > struct drm_framebuffer *old_fb; > > > > + /* Primary plane to expose to userspace if the client sets the 'expose > > + * primary planes' cap. > > + */ > > + struct drm_plane *primary_plane; > > + > > bool enabled; > > > > /* Requested mode from modesetting. */ > > @@ -859,6 +864,7 @@ struct drm_mode_config { > > int num_plane; > > struct list_head plane_list; > > > > + int num_primary_plane; > > int num_crtc; > > struct list_head crtc_list; > > > > @@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev, > > const struct drm_plane_funcs *funcs, > > const uint32_t *formats, uint32_t format_count, > > bool priv); > > +extern int drm_plane_set_primary(struct drm_device *dev, > > + struct drm_plane *plane, struct drm_crtc *crtc, > > + const struct drm_plane_funcs *funcs, > > + const uint32_t *formats, > > + uint32_t format_count); > > extern void drm_plane_cleanup(struct drm_plane *plane); > > extern void drm_plane_force_disable(struct drm_plane *plane); > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 3c9a833..58a2468 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -635,6 +635,14 @@ struct drm_get_cap { > > */ > > #define DRM_CLIENT_CAP_STEREO_3D 1 > > > > +/** > > + * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES > > + * > > + * If set to 1, the DRM core will expose CRTC primary planes along with > > + * sprite/overlay planes. > > + */ > > +#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES 2 > > + > > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > > struct drm_set_client_cap { > > __u64 capability; > > -- > > 1.8.5.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Mar 03, 2014 at 09:45:53AM -0800, Matt Roper wrote: > On Mon, Mar 03, 2014 at 03:47:43PM +0000, Damien Lespiau wrote: > > On Thu, Feb 27, 2014 at 02:14:40PM -0800, Matt Roper wrote: > > > Allow drivers to provide a drm_plane structure corresponding to a CRTC's > > > primary plane. These planes will be included in the plane list for any > > > clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit. > > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > > Two small notes here and comments inside: > > > > - In term of new API enabling and to make the tree bisectable, one needs > > to 1/ do the preparation work 2/ enable the API. In this case, I would > > split up the patch to make the ioctl bits independent and the last > > patch of the series. > > > > - I don't see a point in introducing the complexity to enable sprite and > > cursor planes independently from each other. So before enabling the > > new setcap() ioctl, could we also expose the cursor planes and call > > the capability "universal planes" or similar? > > I'm not sure I follow what you're saying on this second point. Sprites > (or overlays) are already exposed to userspace, so existing clients > expect anything they receive via drmModeGetPlaneResources() to behave in > the traditional manner. If we start throwing cursor planes into that > list without hiding them behind a capability bit, existing userspace try > to blindly use the cursors as full overlays without realizing that they > may carry additional restrictions on size and such, which will lead to > userspace breakage. I mean, I don't see the point of having 2 separate calls to the SET_CAP ioctl() to enable both primary and cursor planes. I think we should have a single cap called "Universal planes" that expose both (which of course means we need to do the same work for cursor plane before the ioctl patch). This way we have fewer corner cases to care about.
Hi On Thu, Feb 27, 2014 at 11:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote: > Allow drivers to provide a drm_plane structure corresponding to a CRTC's > primary plane. These planes will be included in the plane list for any > clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 168 ++++++++++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/drm_ioctl.c | 5 ++ > include/drm/drmP.h | 2 + > include/drm/drm_crtc.h | 11 +++ > include/uapi/drm/drm.h | 8 +++ > 5 files changed, 189 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 35ea15d..21c6d4b 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status) > } > EXPORT_SYMBOL(drm_get_connector_status_name); > > +/* > + * Default set of pixel formats supported by primary planes. This matches the > + * set of formats accepted by the traditional modesetting interfaces; drivers > + * need only provide their own format list if it differs from the default. > + */ > +static const uint32_t default_primary_plane_formats[] = { > + DRM_FORMAT_C8, > + DRM_FORMAT_RGB332, > + DRM_FORMAT_BGR233, > + DRM_FORMAT_XRGB4444, > + DRM_FORMAT_XBGR4444, > + DRM_FORMAT_RGBX4444, > + DRM_FORMAT_BGRX4444, > + DRM_FORMAT_ARGB4444, > + DRM_FORMAT_ABGR4444, > + DRM_FORMAT_RGBA4444, > + DRM_FORMAT_BGRA4444, > + DRM_FORMAT_XRGB1555, > + DRM_FORMAT_XBGR1555, > + DRM_FORMAT_RGBX5551, > + DRM_FORMAT_BGRX5551, > + DRM_FORMAT_ARGB1555, > + DRM_FORMAT_ABGR1555, > + DRM_FORMAT_RGBA5551, > + DRM_FORMAT_BGRA5551, > + DRM_FORMAT_RGB565, > + DRM_FORMAT_BGR565, > + DRM_FORMAT_RGB888, > + DRM_FORMAT_BGR888, > + DRM_FORMAT_XRGB8888, > + DRM_FORMAT_XBGR8888, > + DRM_FORMAT_RGBX8888, > + DRM_FORMAT_BGRX8888, > + DRM_FORMAT_ARGB8888, > + DRM_FORMAT_ABGR8888, > + DRM_FORMAT_RGBA8888, > + DRM_FORMAT_BGRA8888, > + DRM_FORMAT_XRGB2101010, > + DRM_FORMAT_XBGR2101010, > + DRM_FORMAT_RGBX1010102, > + DRM_FORMAT_BGRX1010102, > + DRM_FORMAT_ARGB2101010, > + DRM_FORMAT_ABGR2101010, > + DRM_FORMAT_RGBA1010102, > + DRM_FORMAT_BGRA1010102, > + DRM_FORMAT_YUYV, > + DRM_FORMAT_YVYU, > + DRM_FORMAT_UYVY, > + DRM_FORMAT_VYUY, > + DRM_FORMAT_AYUV, > + DRM_FORMAT_NV12, > + DRM_FORMAT_NV21, > + DRM_FORMAT_NV16, > + DRM_FORMAT_NV61, > + DRM_FORMAT_NV24, > + DRM_FORMAT_NV42, > + DRM_FORMAT_YUV410, > + DRM_FORMAT_YVU410, > + DRM_FORMAT_YUV411, > + DRM_FORMAT_YVU411, > + DRM_FORMAT_YUV420, > + DRM_FORMAT_YVU420, > + DRM_FORMAT_YUV422, > + DRM_FORMAT_YVU422, > + DRM_FORMAT_YUV444, > + DRM_FORMAT_YVU444, > +}; > + As Damien said, I'd drop that list. I don't think we save much by providing a default.. > /** > * drm_get_subpixel_order_name - return a string for a given subpixel enum > * @order: enum of subpixel_order > @@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) > EXPORT_SYMBOL(drm_encoder_cleanup); > > /** > - * drm_plane_init - Initialise a new plane object > + * drm_plane_init - Initialise a new sprite plane object > * @dev: DRM device > * @plane: plane object to init > * @possible_crtcs: bitmask of possible CRTCs > @@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup); > * @format_count: number of elements in @formats > * @priv: plane is private (hidden from userspace)? > * > - * Inits a new object created as base part of a driver plane object. > + * Inits a new object created as base part of a driver plane object. This > + * function should only be called for traditional sprite/overlay planes. > + * Primary planes should be initialized via @drm_plane_set_primary. > * > * RETURNS: > * Zero on success, error code on failure. > @@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > EXPORT_SYMBOL(drm_plane_init); > > /** > + * drm_plane_set_primary - Supply a primary plane for a CRTC > + * @dev DRM device > + * @plane: plane object representing the primary plane > + * @crtc: CRTC this plane is associated with > + * @funcs: callbacks for the new plane > + * @formats: array of supported formats (%DRM_FORMAT_*). If NULL, the > + * default list of formats traditionally supported by modesetting > + * API's will be used and @format_count will be ignored. > + * @format_count: number of elements in @formats > + * > + * Provides a drm_plane representing a CRTC's primary plane. This plane will > + * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES > + * capability bit. > + * > + * RETURNS: > + * Zero on success, error code on failure. > + */ > +int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, > + struct drm_crtc *crtc, > + const struct drm_plane_funcs *funcs, > + const uint32_t *formats, uint32_t format_count) > +{ > + int ret; > + > + drm_modeset_lock_all(dev); > + > + ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE); > + if (ret) > + goto out; > + > + if (formats == NULL) { > + formats = default_primary_plane_formats; > + format_count = ARRAY_SIZE(default_primary_plane_formats); > + } > + > + plane->base.properties = &plane->properties; > + plane->dev = dev; > + plane->funcs = funcs; > + plane->format_types = kmalloc(sizeof(uint32_t) * format_count, > + GFP_KERNEL); > + if (!plane->format_types) { > + DRM_DEBUG_KMS("out of memory when setting up primary plane\n"); > + drm_mode_object_put(dev, &plane->base); > + ret = -ENOMEM; > + goto out; > + } > + > + memcpy(plane->format_types, formats, format_count * sizeof(uint32_t)); > + plane->format_count = format_count; > + plane->possible_crtcs = drm_crtc_mask(crtc); > + > + /* > + * Primary planes are not added to the general plane list, only linked > + * to their CRTC. They will only be advertised to userspace clients > + * that indicate support. > + */ > + crtc->primary_plane = plane; > + dev->mode_config.num_primary_plane++; > + INIT_LIST_HEAD(&plane->head); > + > + out: > + drm_modeset_unlock_all(dev); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_plane_set_primary); > + > +/** > * drm_plane_cleanup - Clean up the core plane usage > * @plane: plane to cleanup > * > @@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, > struct drm_mode_get_plane_res *plane_resp = data; > struct drm_mode_config *config; > struct drm_plane *plane; > + struct drm_crtc *crtc; > uint32_t __user *plane_ptr; > int copied = 0, ret = 0; > + unsigned num_planes; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EINVAL; > @@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, > drm_modeset_lock_all(dev); > config = &dev->mode_config; > > + num_planes = config->num_plane; > + if (file_priv->expose_primary_planes) > + num_planes += config->num_primary_plane; > + > /* > * This ioctl is called twice, once to determine how much space is > * needed, and the 2nd time to fill it. > */ > - if (config->num_plane && > - (plane_resp->count_planes >= config->num_plane)) { > + if (num_planes && > + (plane_resp->count_planes >= num_planes)) { > plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr; > > + if (file_priv->expose_primary_planes) { > + list_for_each_entry(crtc, &config->crtc_list, head) { > + if (!crtc->primary_plane) > + continue; > + > + if (put_user(crtc->primary_plane->base.id, > + plane_ptr + copied)) { > + ret = -EFAULT; > + goto out; > + } > + copied++; > + } > + } > + How can user-space distinguish primary-planes from others? With atomic-modesetting I can understand that there's no need to distinguish them, but without it, we need to know which planes not to use. Same problem for the possible_crtc field. The plane might work with other CRTCs, but for legacy modesetting we don't care as we have no API to use it. I'd be nice to see somehow which primary plane is assigned to which crtc. But maybe this all is just not intended to be used without atomic-modesetting. I like the proposal! David > list_for_each_entry(plane, &config->plane_list, head) { > if (put_user(plane->base.id, plane_ptr + copied)) { > ret = -EFAULT; > @@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, > copied++; > } > } > - plane_resp->count_planes = config->num_plane; > + plane_resp->count_planes = num_planes; > > out: > drm_modeset_unlock_all(dev); > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index dffc836..03579d6 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) > return -EINVAL; > file_priv->stereo_allowed = req->value; > break; > + case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES: > + if (req->value > 1) > + return -EINVAL; > + file_priv->expose_primary_planes = req->value; > + break; > default: > return -EINVAL; > } > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 04a7f31..997fa29 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -437,6 +437,8 @@ struct drm_file { > unsigned is_master :1; /* this file private is a master for a minor */ > /* true when the client has asked us to expose stereo 3D mode flags */ > unsigned stereo_allowed :1; > + /* true if client understands CRTC primary planes in the plane list */ > + unsigned expose_primary_planes:1; > > struct pid *pid; > kuid_t uid; > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index ce9ee60..33a955b 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -429,6 +429,11 @@ struct drm_crtc { > * by drm_mode_set_config_internal to implement correct refcounting. */ > struct drm_framebuffer *old_fb; > > + /* Primary plane to expose to userspace if the client sets the 'expose > + * primary planes' cap. > + */ > + struct drm_plane *primary_plane; > + > bool enabled; > > /* Requested mode from modesetting. */ > @@ -859,6 +864,7 @@ struct drm_mode_config { > int num_plane; > struct list_head plane_list; > > + int num_primary_plane; > int num_crtc; > struct list_head crtc_list; > > @@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev, > const struct drm_plane_funcs *funcs, > const uint32_t *formats, uint32_t format_count, > bool priv); > +extern int drm_plane_set_primary(struct drm_device *dev, > + struct drm_plane *plane, struct drm_crtc *crtc, > + const struct drm_plane_funcs *funcs, > + const uint32_t *formats, > + uint32_t format_count); > extern void drm_plane_cleanup(struct drm_plane *plane); > extern void drm_plane_force_disable(struct drm_plane *plane); > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 3c9a833..58a2468 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -635,6 +635,14 @@ struct drm_get_cap { > */ > #define DRM_CLIENT_CAP_STEREO_3D 1 > > +/** > + * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES > + * > + * If set to 1, the DRM core will expose CRTC primary planes along with > + * sprite/overlay planes. > + */ > +#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES 2 > + > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > struct drm_set_client_cap { > __u64 capability; > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Mar 03, 2014 at 07:24:04PM +0100, David Herrmann wrote: > How can user-space distinguish primary-planes from others? With > atomic-modesetting I can understand that there's no need to > distinguish them, but without it, we need to know which planes not to > use. Same problem for the possible_crtc field. The plane might work > with other CRTCs, but for legacy modesetting we don't care as we have > no API to use it. > > I'd be nice to see somehow which primary plane is assigned to which > crtc. But maybe this all is just not intended to be used without > atomic-modesetting. > > I like the proposal! My take on this is that if the client has enable the new capbility, it should not use the legacy cursor ioctls, nor should it specify a framebuffer in the setcrtc ioctl. In fact I'd just return an error if it does that. But we still have the issue of figuring out what each plane can and can't do. We need to expose that somehow, ideally in some form that's not too hardware specific. So some kind of plane caps would be needed, either as a separate ioctl or as read-only properties.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 35ea15d..21c6d4b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status) } EXPORT_SYMBOL(drm_get_connector_status_name); +/* + * Default set of pixel formats supported by primary planes. This matches the + * set of formats accepted by the traditional modesetting interfaces; drivers + * need only provide their own format list if it differs from the default. + */ +static const uint32_t default_primary_plane_formats[] = { + DRM_FORMAT_C8, + DRM_FORMAT_RGB332, + DRM_FORMAT_BGR233, + DRM_FORMAT_XRGB4444, + DRM_FORMAT_XBGR4444, + DRM_FORMAT_RGBX4444, + DRM_FORMAT_BGRX4444, + DRM_FORMAT_ARGB4444, + DRM_FORMAT_ABGR4444, + DRM_FORMAT_RGBA4444, + DRM_FORMAT_BGRA4444, + DRM_FORMAT_XRGB1555, + DRM_FORMAT_XBGR1555, + DRM_FORMAT_RGBX5551, + DRM_FORMAT_BGRX5551, + DRM_FORMAT_ARGB1555, + DRM_FORMAT_ABGR1555, + DRM_FORMAT_RGBA5551, + DRM_FORMAT_BGRA5551, + DRM_FORMAT_RGB565, + DRM_FORMAT_BGR565, + DRM_FORMAT_RGB888, + DRM_FORMAT_BGR888, + DRM_FORMAT_XRGB8888, + DRM_FORMAT_XBGR8888, + DRM_FORMAT_RGBX8888, + DRM_FORMAT_BGRX8888, + DRM_FORMAT_ARGB8888, + DRM_FORMAT_ABGR8888, + DRM_FORMAT_RGBA8888, + DRM_FORMAT_BGRA8888, + DRM_FORMAT_XRGB2101010, + DRM_FORMAT_XBGR2101010, + DRM_FORMAT_RGBX1010102, + DRM_FORMAT_BGRX1010102, + DRM_FORMAT_ARGB2101010, + DRM_FORMAT_ABGR2101010, + DRM_FORMAT_RGBA1010102, + DRM_FORMAT_BGRA1010102, + DRM_FORMAT_YUYV, + DRM_FORMAT_YVYU, + DRM_FORMAT_UYVY, + DRM_FORMAT_VYUY, + DRM_FORMAT_AYUV, + DRM_FORMAT_NV12, + DRM_FORMAT_NV21, + DRM_FORMAT_NV16, + DRM_FORMAT_NV61, + DRM_FORMAT_NV24, + DRM_FORMAT_NV42, + DRM_FORMAT_YUV410, + DRM_FORMAT_YVU410, + DRM_FORMAT_YUV411, + DRM_FORMAT_YVU411, + DRM_FORMAT_YUV420, + DRM_FORMAT_YVU420, + DRM_FORMAT_YUV422, + DRM_FORMAT_YVU422, + DRM_FORMAT_YUV444, + DRM_FORMAT_YVU444, +}; + /** * drm_get_subpixel_order_name - return a string for a given subpixel enum * @order: enum of subpixel_order @@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) EXPORT_SYMBOL(drm_encoder_cleanup); /** - * drm_plane_init - Initialise a new plane object + * drm_plane_init - Initialise a new sprite plane object * @dev: DRM device * @plane: plane object to init * @possible_crtcs: bitmask of possible CRTCs @@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup); * @format_count: number of elements in @formats * @priv: plane is private (hidden from userspace)? * - * Inits a new object created as base part of a driver plane object. + * Inits a new object created as base part of a driver plane object. This + * function should only be called for traditional sprite/overlay planes. + * Primary planes should be initialized via @drm_plane_set_primary. * * RETURNS: * Zero on success, error code on failure. @@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, EXPORT_SYMBOL(drm_plane_init); /** + * drm_plane_set_primary - Supply a primary plane for a CRTC + * @dev DRM device + * @plane: plane object representing the primary plane + * @crtc: CRTC this plane is associated with + * @funcs: callbacks for the new plane + * @formats: array of supported formats (%DRM_FORMAT_*). If NULL, the + * default list of formats traditionally supported by modesetting + * API's will be used and @format_count will be ignored. + * @format_count: number of elements in @formats + * + * Provides a drm_plane representing a CRTC's primary plane. This plane will + * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES + * capability bit. + * + * RETURNS: + * Zero on success, error code on failure. + */ +int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, + struct drm_crtc *crtc, + const struct drm_plane_funcs *funcs, + const uint32_t *formats, uint32_t format_count) +{ + int ret; + + drm_modeset_lock_all(dev); + + ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE); + if (ret) + goto out; + + if (formats == NULL) { + formats = default_primary_plane_formats; + format_count = ARRAY_SIZE(default_primary_plane_formats); + } + + plane->base.properties = &plane->properties; + plane->dev = dev; + plane->funcs = funcs; + plane->format_types = kmalloc(sizeof(uint32_t) * format_count, + GFP_KERNEL); + if (!plane->format_types) { + DRM_DEBUG_KMS("out of memory when setting up primary plane\n"); + drm_mode_object_put(dev, &plane->base); + ret = -ENOMEM; + goto out; + } + + memcpy(plane->format_types, formats, format_count * sizeof(uint32_t)); + plane->format_count = format_count; + plane->possible_crtcs = drm_crtc_mask(crtc); + + /* + * Primary planes are not added to the general plane list, only linked + * to their CRTC. They will only be advertised to userspace clients + * that indicate support. + */ + crtc->primary_plane = plane; + dev->mode_config.num_primary_plane++; + INIT_LIST_HEAD(&plane->head); + + out: + drm_modeset_unlock_all(dev); + + return ret; +} +EXPORT_SYMBOL(drm_plane_set_primary); + +/** * drm_plane_cleanup - Clean up the core plane usage * @plane: plane to cleanup * @@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, struct drm_mode_get_plane_res *plane_resp = data; struct drm_mode_config *config; struct drm_plane *plane; + struct drm_crtc *crtc; uint32_t __user *plane_ptr; int copied = 0, ret = 0; + unsigned num_planes; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, drm_modeset_lock_all(dev); config = &dev->mode_config; + num_planes = config->num_plane; + if (file_priv->expose_primary_planes) + num_planes += config->num_primary_plane; + /* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ - if (config->num_plane && - (plane_resp->count_planes >= config->num_plane)) { + if (num_planes && + (plane_resp->count_planes >= num_planes)) { plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr; + if (file_priv->expose_primary_planes) { + list_for_each_entry(crtc, &config->crtc_list, head) { + if (!crtc->primary_plane) + continue; + + if (put_user(crtc->primary_plane->base.id, + plane_ptr + copied)) { + ret = -EFAULT; + goto out; + } + copied++; + } + } + list_for_each_entry(plane, &config->plane_list, head) { if (put_user(plane->base.id, plane_ptr + copied)) { ret = -EFAULT; @@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data, copied++; } } - plane_resp->count_planes = config->num_plane; + plane_resp->count_planes = num_planes; out: drm_modeset_unlock_all(dev); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index dffc836..03579d6 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) return -EINVAL; file_priv->stereo_allowed = req->value; break; + case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES: + if (req->value > 1) + return -EINVAL; + file_priv->expose_primary_planes = req->value; + break; default: return -EINVAL; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 04a7f31..997fa29 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -437,6 +437,8 @@ struct drm_file { unsigned is_master :1; /* this file private is a master for a minor */ /* true when the client has asked us to expose stereo 3D mode flags */ unsigned stereo_allowed :1; + /* true if client understands CRTC primary planes in the plane list */ + unsigned expose_primary_planes:1; struct pid *pid; kuid_t uid; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index ce9ee60..33a955b 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -429,6 +429,11 @@ struct drm_crtc { * by drm_mode_set_config_internal to implement correct refcounting. */ struct drm_framebuffer *old_fb; + /* Primary plane to expose to userspace if the client sets the 'expose + * primary planes' cap. + */ + struct drm_plane *primary_plane; + bool enabled; /* Requested mode from modesetting. */ @@ -859,6 +864,7 @@ struct drm_mode_config { int num_plane; struct list_head plane_list; + int num_primary_plane; int num_crtc; struct list_head crtc_list; @@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev, const struct drm_plane_funcs *funcs, const uint32_t *formats, uint32_t format_count, bool priv); +extern int drm_plane_set_primary(struct drm_device *dev, + struct drm_plane *plane, struct drm_crtc *crtc, + const struct drm_plane_funcs *funcs, + const uint32_t *formats, + uint32_t format_count); extern void drm_plane_cleanup(struct drm_plane *plane); extern void drm_plane_force_disable(struct drm_plane *plane); diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3c9a833..58a2468 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -635,6 +635,14 @@ struct drm_get_cap { */ #define DRM_CLIENT_CAP_STEREO_3D 1 +/** + * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES + * + * If set to 1, the DRM core will expose CRTC primary planes along with + * sprite/overlay planes. + */ +#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES 2 + /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ struct drm_set_client_cap { __u64 capability;
Allow drivers to provide a drm_plane structure corresponding to a CRTC's primary plane. These planes will be included in the plane list for any clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_crtc.c | 168 ++++++++++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/drm_ioctl.c | 5 ++ include/drm/drmP.h | 2 + include/drm/drm_crtc.h | 11 +++ include/uapi/drm/drm.h | 8 +++ 5 files changed, 189 insertions(+), 5 deletions(-)