[PATCH 2/4] drm: Add plane type property

[Rob Clark]

On Thu, Feb 27, 2014 at 6:24 PM, Matt Roper <matthew.d.roper at 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 at 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 at 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