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

[Matt Roper <matthew.d.roper@intel.com>]

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

-- 
Matt Roper
Graphics Software Engineer
ISG Platform Enabling & Development
Intel Corporation
(916) 356-2795