| Message ID | 1393539283-5901-3-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote: > Add a plane type property to allow userspace to distinguish > sprite/overlay planes from primary planes. In the future we may extend > this to cover cursor planes as well. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 1 + > include/uapi/drm/drm_mode.h | 3 +++ > 3 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 21c6d4b..1032eaf 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = > > DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) > > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] = > +{ > + { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" }, I'm not the *hugest* fan of using the name "sprite".. at least that too me implies sort of a subset of possible functionality of a plane.. > + { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" }, > +}; > + > +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list) > + > /* > * Optional properties > */ > @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > INIT_LIST_HEAD(&plane->head); > } > > + drm_object_attach_property(&plane->base, > + dev->mode_config.plane_type_property, > + DRM_MODE_PLANE_TYPE_SPRITE); > + > out: > drm_modeset_unlock_all(dev); > > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, fwiw, this comment probably belongs in #1/4 but: you probably don't need to introduce drm_plane_set_primary().. instead you could just rename the 'bool priv' to 'bool prim'. I think there are just three drivers using primary planes.. I'm not 100% sure about exynos, but both omap and msm, the private plane == primary plane. At least it was the intention to morph that into primary planes. Anyways, other than that I like the patchset. Hopefully I should get to rebasing the atomic patches real soon now, so I'll try rebasing on top of this and see how it goes. BR, -R > dev->mode_config.num_primary_plane++; > INIT_LIST_HEAD(&plane->head); > > + drm_object_attach_property(&plane->base, > + dev->mode_config.plane_type_property, > + DRM_MODE_PLANE_TYPE_PRIMARY); > + > out: > drm_modeset_unlock_all(dev); > > @@ -1236,6 +1252,21 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev) > return 0; > } > > +static int drm_mode_create_standard_plane_properties(struct drm_device *dev) > +{ > + struct drm_property *type; > + > + /* > + * Standard properties (apply to all planes) > + */ > + type = drm_property_create_enum(dev, 0, > + "TYPE", drm_plane_type_enum_list, > + ARRAY_SIZE(drm_plane_type_enum_list)); > + dev->mode_config.plane_type_property = type; > + > + return 0; > +} > + > /** > * drm_mode_create_dvi_i_properties - create DVI-I specific connector properties > * @dev: DRM device > @@ -4211,6 +4242,7 @@ void drm_mode_config_init(struct drm_device *dev) > > drm_modeset_lock_all(dev); > drm_mode_create_standard_connector_properties(dev); > + drm_mode_create_standard_plane_properties(dev); > drm_modeset_unlock_all(dev); > > /* Just to be sure */ > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 33a955b..d25cd9c 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -884,6 +884,7 @@ struct drm_mode_config { > struct list_head property_blob_list; > struct drm_property *edid_property; > struct drm_property *dpms_property; > + struct drm_property *plane_type_property; > > /* DVI-I properties */ > struct drm_property *dvi_i_subconnector_property; > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index f104c26..c19705b 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -496,4 +496,7 @@ struct drm_mode_destroy_dumb { > uint32_t handle; > }; > > +#define DRM_MODE_PLANE_TYPE_SPRITE 0 > +#define DRM_MODE_PLANE_TYPE_PRIMARY 1 > + > #endif > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote: > On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote: > > Add a plane type property to allow userspace to distinguish > > sprite/overlay planes from primary planes. In the future we may extend > > this to cover cursor planes as well. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++++++++++++++++ > > include/drm/drm_crtc.h | 1 + > > include/uapi/drm/drm_mode.h | 3 +++ > > 3 files changed, 36 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 21c6d4b..1032eaf 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = > > > > DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) > > > > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] = > > +{ > > + { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" }, > > I'm not the *hugest* fan of using the name "sprite".. at least that > too me implies sort of a subset of possible functionality of a plane.. Any suggestions on a better name? Maybe call them "traditional" planes and then just give new names to the other types (primary, cursor) that we wind up exposing when appropriate client caps are set? > > > + { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" }, > > +}; > > + > > +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list) > > + > > /* > > * Optional properties > > */ > > @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > > INIT_LIST_HEAD(&plane->head); > > } > > > > + drm_object_attach_property(&plane->base, > > + dev->mode_config.plane_type_property, > > + DRM_MODE_PLANE_TYPE_SPRITE); > > + > > out: > > drm_modeset_unlock_all(dev); > > > > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, > > > fwiw, this comment probably belongs in #1/4 but: > > you probably don't need to introduce drm_plane_set_primary().. > instead you could just rename the 'bool priv' to 'bool prim'. I think > there are just three drivers using primary planes.. I'm not 100% sure > about exynos, but both omap and msm, the private plane == primary > plane. At least it was the intention to morph that into primary > planes. I'd like to handle cursors with this eventually as well, so I'm not sure whether just changing the meaning of priv by itself will get us everything we need. It seems like we probably need to provide a whole lot more information about the capabilities and limitations of each plane at drm_plane_init() and then expose those all as plane properties so that userspace knows what it can and can't do. In theory we could expose cursor planes exactly the same way we expose "traditional" planes today as long as we made sufficient plane properties available to userspace to describe the min/max size limitations and such. > > Anyways, other than that I like the patchset. Hopefully I should get > to rebasing the atomic patches real soon now, so I'll try rebasing on > top of this and see how it goes. > > BR, > -R Sounds good; thanks for the review. Matt
On Thu, Feb 27, 2014 at 6:24 PM, Matt Roper <matthew.d.roper@intel.com> wrote: > On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote: >> On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote: >> > Add a plane type property to allow userspace to distinguish >> > sprite/overlay planes from primary planes. In the future we may extend >> > this to cover cursor planes as well. >> > >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >> > --- >> > drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++++++++++++++++ >> > include/drm/drm_crtc.h | 1 + >> > include/uapi/drm/drm_mode.h | 3 +++ >> > 3 files changed, 36 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> > index 21c6d4b..1032eaf 100644 >> > --- a/drivers/gpu/drm/drm_crtc.c >> > +++ b/drivers/gpu/drm/drm_crtc.c >> > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = >> > >> > DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) >> > >> > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] = >> > +{ >> > + { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" }, >> >> I'm not the *hugest* fan of using the name "sprite".. at least that >> too me implies sort of a subset of possible functionality of a plane.. > > Any suggestions on a better name? Maybe call them "traditional" planes > and then just give new names to the other types (primary, cursor) that > we wind up exposing when appropriate client caps are set? Maybe just "overlay"? I'm not sure, I was hoping that by mentioning it, that would trigger an idea in someone ;-) >> >> > + { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" }, >> > +}; >> > + >> > +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list) >> > + >> > /* >> > * Optional properties >> > */ >> > @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, >> > INIT_LIST_HEAD(&plane->head); >> > } >> > >> > + drm_object_attach_property(&plane->base, >> > + dev->mode_config.plane_type_property, >> > + DRM_MODE_PLANE_TYPE_SPRITE); >> > + >> > out: >> > drm_modeset_unlock_all(dev); >> > >> > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, >> >> >> fwiw, this comment probably belongs in #1/4 but: >> >> you probably don't need to introduce drm_plane_set_primary().. >> instead you could just rename the 'bool priv' to 'bool prim'. I think >> there are just three drivers using primary planes.. I'm not 100% sure >> about exynos, but both omap and msm, the private plane == primary >> plane. At least it was the intention to morph that into primary >> planes. > > I'd like to handle cursors with this eventually as well, so I'm not sure > whether just changing the meaning of priv by itself will get us > everything we need. It seems like we probably need to provide a whole > lot more information about the capabilities and limitations of each > plane at drm_plane_init() and then expose those all as plane > properties so that userspace knows what it can and can't do. In theory > we could expose cursor planes exactly the same way we expose > "traditional" planes today as long as we made sufficient plane > properties available to userspace to describe the min/max size > limitations and such. We could also just go the opposite direction, ie. keep _set_primary() and drop the 'priv' arg.. I don't really mind too much either way, but the 'private' plane stuff was intended to eventually be exposed to userspace.. so if we call it primary now (which is a much better name, IMO), we should clean out the remaining references to 'private'. BR, -R >> >> Anyways, other than that I like the patchset. Hopefully I should get >> to rebasing the atomic patches real soon now, so I'll try rebasing on >> top of this and see how it goes. >> >> BR, >> -R > > Sounds good; thanks for the review. > > > Matt > > -- > Matt Roper > Graphics Software Engineer > ISG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On Thu, Feb 27, 2014 at 11:03:07PM -0500, Rob Clark wrote: > >> > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, > >> > >> > >> fwiw, this comment probably belongs in #1/4 but: > >> > >> you probably don't need to introduce drm_plane_set_primary().. > >> instead you could just rename the 'bool priv' to 'bool prim'. I think > >> there are just three drivers using primary planes.. I'm not 100% sure > >> about exynos, but both omap and msm, the private plane == primary > >> plane. At least it was the intention to morph that into primary > >> planes. > > > > I'd like to handle cursors with this eventually as well, so I'm not sure > > whether just changing the meaning of priv by itself will get us > > everything we need. It seems like we probably need to provide a whole > > lot more information about the capabilities and limitations of each > > plane at drm_plane_init() and then expose those all as plane > > properties so that userspace knows what it can and can't do. In theory > > we could expose cursor planes exactly the same way we expose > > "traditional" planes today as long as we made sufficient plane > > properties available to userspace to describe the min/max size > > limitations and such. > > We could also just go the opposite direction, ie. keep _set_primary() > and drop the 'priv' arg.. I don't really mind too much either way, but > the 'private' plane stuff was intended to eventually be exposed to > userspace.. so if we call it primary now (which is a much better > name, IMO), we should clean out the remaining references to 'private'. Ah, I had the same comment in patch 1/4. Why not have drm_init_plane() take the type of plane as the last argument then? (instead of bool primary, just the plane_type enum, primary, "sprite", cursor).
On Thu, Feb 27, 2014 at 03:24:08PM -0800, Matt Roper wrote: > On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote: > > On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote: > > > Add a plane type property to allow userspace to distinguish > > > sprite/overlay planes from primary planes. In the future we may extend > > > this to cover cursor planes as well. > > > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > > --- > > > drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++++++++++++++++ > > > include/drm/drm_crtc.h | 1 + > > > include/uapi/drm/drm_mode.h | 3 +++ > > > 3 files changed, 36 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > index 21c6d4b..1032eaf 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = > > > > > > DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) > > > > > > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] = > > > +{ > > > + { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" }, > > > > I'm not the *hugest* fan of using the name "sprite".. at least that > > too me implies sort of a subset of possible functionality of a plane.. > > Any suggestions on a better name? Maybe call them "traditional" planes > and then just give new names to the other types (primary, cursor) that > we wind up exposing when appropriate client caps are set? What about "secondary" for any plane exposed which doesn't match one of the special-purpose planes for backwards compat? We'd then have "primary" (fixed to a crtc and used for legacy setCrtc/pageflips), "cursor" (again fixed to a crtc for use by the legacy setcurso ioctl) and a pile of secondary planes without special meaning attached to them. -Daniel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 21c6d4b..1032eaf 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] = DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list) +static const struct drm_prop_enum_list drm_plane_type_enum_list[] = +{ + { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" }, + { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" }, +}; + +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list) + /* * Optional properties */ @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, INIT_LIST_HEAD(&plane->head); } + drm_object_attach_property(&plane->base, + dev->mode_config.plane_type_property, + DRM_MODE_PLANE_TYPE_SPRITE); + out: drm_modeset_unlock_all(dev); @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane, dev->mode_config.num_primary_plane++; INIT_LIST_HEAD(&plane->head); + drm_object_attach_property(&plane->base, + dev->mode_config.plane_type_property, + DRM_MODE_PLANE_TYPE_PRIMARY); + out: drm_modeset_unlock_all(dev); @@ -1236,6 +1252,21 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev) return 0; } +static int drm_mode_create_standard_plane_properties(struct drm_device *dev) +{ + struct drm_property *type; + + /* + * Standard properties (apply to all planes) + */ + type = drm_property_create_enum(dev, 0, + "TYPE", drm_plane_type_enum_list, + ARRAY_SIZE(drm_plane_type_enum_list)); + dev->mode_config.plane_type_property = type; + + return 0; +} + /** * drm_mode_create_dvi_i_properties - create DVI-I specific connector properties * @dev: DRM device @@ -4211,6 +4242,7 @@ void drm_mode_config_init(struct drm_device *dev) drm_modeset_lock_all(dev); drm_mode_create_standard_connector_properties(dev); + drm_mode_create_standard_plane_properties(dev); drm_modeset_unlock_all(dev); /* Just to be sure */ diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 33a955b..d25cd9c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -884,6 +884,7 @@ struct drm_mode_config { struct list_head property_blob_list; struct drm_property *edid_property; struct drm_property *dpms_property; + struct drm_property *plane_type_property; /* DVI-I properties */ struct drm_property *dvi_i_subconnector_property; diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index f104c26..c19705b 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -496,4 +496,7 @@ struct drm_mode_destroy_dumb { uint32_t handle; }; +#define DRM_MODE_PLANE_TYPE_SPRITE 0 +#define DRM_MODE_PLANE_TYPE_PRIMARY 1 + #endif
Add a plane type property to allow userspace to distinguish sprite/overlay planes from primary planes. In the future we may extend this to cover cursor planes as well. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/drm_crtc.c | 32 ++++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 1 + include/uapi/drm/drm_mode.h | 3 +++ 3 files changed, 36 insertions(+)