From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFCv3 03/14] drm: Add primary plane helpers Date: Wed, 19 Mar 2014 12:50:07 +0100 Message-ID: <20140319115007.GU30571@phenom.ffwll.local> References: <1395188579-17191-1-git-send-email-matthew.d.roper@intel.com> <1395188579-17191-4-git-send-email-matthew.d.roper@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f54.google.com (mail-ee0-f54.google.com [74.125.83.54]) by gabe.freedesktop.org (Postfix) with ESMTP id 560768EC4C for ; Wed, 19 Mar 2014 04:50:13 -0700 (PDT) Received: by mail-ee0-f54.google.com with SMTP id d49so6525273eek.13 for ; Wed, 19 Mar 2014 04:50:12 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1395188579-17191-4-git-send-email-matthew.d.roper@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Matt Roper Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Tue, Mar 18, 2014 at 05:22:48PM -0700, Matt Roper wrote: > When we expose non-overlay planes to userspace, they will become > accessible via standard userspace plane API's. We should be able to > handle the standard plane operations against primary planes in a generic > way via the page flip handler and modeset handler. > > Drivers that can program primary planes more efficiently, that want to > use their own primary plane structure to track additional information, > or that don't have the limitations assumed by the helpers are free to > provide their own implementation of some or all of these handlers. > > Signed-off-by: Matt Roper > --- One more below ... > +/** > + * drm_primary_helper_disable() - Helper for primary plane disable > + * @plane: plane to disable > + * > + * Provides a default plane disable handler for primary planes. This is handler > + * is called in response to a userspace SetPlane operation on the plane with a > + * NULL framebuffer parameter. We call the driver's modeset handler with a NULL > + * framebuffer to disable the CRTC. > + * > + * Note that some hardware may be able to disable the primary plane without > + * disabling the whole CRTC. Drivers for such hardware should provide their > + * own disable handler that disables just the primary plane (and they'll likely > + * need to provide their own update handler as well to properly re-enable a > + * disabled primary plane). > + * > + * RETURNS: > + * Zero on success, error code on failure > + */ > +int drm_primary_helper_disable(struct drm_plane *plane) > +{ > + struct drm_mode_set set = { > + .crtc = plane->crtc, > + .fb = NULL, > + }; > + > + if (plane->crtc == NULL || plane->fb == NULL) > + /* Already disabled */ > + return 0; I think we should have a check here if any other plane is enabled (including the cursor plane), and fail the plane disabling with -EBUSY. Otherwise new userspace has no way to figure out whether the driver is updated already or not. > + > + return plane->crtc->funcs->set_config(&set); Again I think you need to use set_config_internal to have correct fb refcounting. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch