From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Clark Subject: Re: [PATCH 2/4] drm: Add plane type property Date: Thu, 27 Feb 2014 23:03:07 -0500 Message-ID: References: <1393539283-5901-1-git-send-email-matthew.d.roper@intel.com> <1393539283-5901-3-git-send-email-matthew.d.roper@intel.com> <20140227232408.GU27672@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f174.google.com (mail-ie0-f174.google.com [209.85.223.174]) by gabe.freedesktop.org (Postfix) with ESMTP id 38FE8FBA43 for ; Thu, 27 Feb 2014 20:03:08 -0800 (PST) Received: by mail-ie0-f174.google.com with SMTP id rp18so2973255iec.33 for ; Thu, 27 Feb 2014 20:03:07 -0800 (PST) In-Reply-To: <20140227232408.GU27672@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Matt Roper Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org 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(&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