[PATCH 2/4] drm: Add plane type property
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
> > 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
> > > ---
> > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 2/4] drm: Add plane type property
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). -- Damien
[PATCH 2/4] drm: Add plane type property
On Thu, Feb 27, 2014 at 6:24 PM, 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
>> 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
>> > ---
>> > 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(>head);
>> > }
>> >
>> > + drm_object_attach_property(>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
[PATCH 2/4] drm: Add plane type property
On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper
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
> ---
> 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(>head);
> }
>
> + drm_object_attach_property(>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(>head);
>
> + drm_object_attach_property(>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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/4] drm: Add plane type property
On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote:
> On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper
> 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
> > ---
> > 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(>head);
> > }
> >
> > + drm_object_attach_property(>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
--
Matt Roper
Graphics Software Engineer
ISG Platform Enabling & Development
Intel Corporation
(916) 356-2795
[PATCH 2/4] drm: Add plane type property
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
---
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" },
+ { 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(>head);
}
+ drm_object_attach_property(>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(>head);
+ drm_object_attach_property(>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
