From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/plane-helper: Don't fake-implement primary plane disabling Date: Tue, 15 Apr 2014 13:22:24 +0300 Message-ID: <20140415102224.GR18465@intel.com> References: <1397548963-18189-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1397548963-18189-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Intel Graphics Development , DRI Development List-Id: intel-gfx@lists.freedesktop.org On Tue, Apr 15, 2014 at 10:02:43AM +0200, Daniel Vetter wrote: > After thinking about this topic a bit more I've reached the conclusion > that implementing this doesn't make sense: > = > - The locking is all wrong: set_config(NULL) will also unlink encoders > and connectors, but those links are protected with the mode_config > mutex. In the ->disable_plane callback we only hold all modeset > locks, but eventually we want to switch to just grabbing the > per-crtc (and maybe per-plane) locks as needed, maybe based on > ww_mutexes. Having a callback which absolutely needs all modeset > locks is bad for this conversion. > = > Note that the same isn't true for the provided ->update_plane since > we've audited the crtc helpers to make sure that not encoder or > connector links are changed. > = > - There's no way to re-enable the plane with an ->update_plane: The > connectors/encoder links are lost and so we can't re-enable the > CRTC. Even without that issue the driver might have reassigned some > shared resources (as opposed to e.g. DPMS off, where drivers are not > allowed to do that to make sure the CRTC can be enabled again). > = > - The semantics don't make much sense: Userspace asked to scan out > black (or some other color if the driver supports a background > color), not that the screen be disabled. > = > - Implementing proper primary plane support (i.e. actually disabling > the primary plane without disabling the CRTC) is really simple, at > least if all the hw needs is flipping a bit. The big task is > auditing all the interactions with other ioctls when the CRTC is on > but there's no primary plane (e.g. pageflips). And some of that work > still needs to be done. > = > Cc: Matt Roper > Signed-off-by: Daniel Vetter Reviewed-by: Ville Syrj=E4l=E4 Although I'm not sure EINVAL is the best choice here. Maybe ENOSYS? > --- > drivers/gpu/drm/drm_plane_helper.c | 33 +++++---------------------------- > 1 file changed, 5 insertions(+), 28 deletions(-) > = > diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_pla= ne_helper.c > index 9263fd5efa58..9540ff9f97fe 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -205,9 +205,9 @@ EXPORT_SYMBOL(drm_primary_helper_update); > * > * 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 wit= h a NULL > - * framebuffer to disable the CRTC if no other planes are currently enab= led. > - * If other planes are still enabled on the same CRTC, we return -EBUSY. > + * NULL framebuffer parameter. It unconditionally fails the disable cal= l with > + * -EINVAL the only way to disable the primary plane without driver supp= ort is > + * to disable the entier CRTC. Which does not match the plane ->disable = hook. > * > * Note that some hardware may be able to disable the primary plane with= out > * disabling the whole CRTC. Drivers for such hardware should provide t= heir > @@ -216,34 +216,11 @@ EXPORT_SYMBOL(drm_primary_helper_update); > * disabled primary plane). > * > * RETURNS: > - * Zero on success, error code on failure > + * Unconditionally returns -EINVAL. > */ > int drm_primary_helper_disable(struct drm_plane *plane) > { > - struct drm_plane *p; > - struct drm_mode_set set =3D { > - .crtc =3D plane->crtc, > - .fb =3D NULL, > - }; > - > - if (plane->crtc =3D=3D NULL || plane->fb =3D=3D NULL) > - /* Already disabled */ > - return 0; > - > - list_for_each_entry(p, &plane->dev->mode_config.plane_list, head) > - if (p !=3D plane && p->fb) { > - DRM_DEBUG_KMS("Cannot disable primary plane while other planes are st= ill active on CRTC.\n"); > - return -EBUSY; > - } > - > - /* > - * N.B. We call set_config() directly here rather than > - * drm_mode_set_config_internal() since drm_mode_setplane() already > - * handles the basic refcounting and we don't need the special > - * cross-CRTC refcounting (no chance of stealing connectors from > - * other CRTC's with this update). > - */ > - return plane->crtc->funcs->set_config(&set); > + return -EINVAL; > } > EXPORT_SYMBOL(drm_primary_helper_disable); > = > -- = > 1.9.2 > = > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- = Ville Syrj=E4l=E4 Intel OTC