From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [RFC 05/12] drm: Create primary minor only if mode-setting is supported Date: Tue, 25 Feb 2014 16:36:32 +0100 Message-ID: <20140225153630.GC723@ulmo.nvidia.com> References: <1392969328-23133-1-git-send-email-thierry.reding@gmail.com> <1392969328-23133-6-git-send-email-thierry.reding@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1074205720==" Return-path: Received: from mail-bk0-f42.google.com (mail-bk0-f42.google.com [209.85.214.42]) by gabe.freedesktop.org (Postfix) with ESMTP id 98AF4FAD66 for ; Tue, 25 Feb 2014 07:36:35 -0800 (PST) Received: by mail-bk0-f42.google.com with SMTP id mx12so295216bkb.1 for ; Tue, 25 Feb 2014 07:36:34 -0800 (PST) In-Reply-To: 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: David Herrmann Cc: Daniel Vetter , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1074205720== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ncSAzJYg3Aa9+CRW" Content-Disposition: inline --ncSAzJYg3Aa9+CRW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Feb 24, 2014 at 11:39:33AM +0100, David Herrmann wrote: > Hi >=20 > On Fri, Feb 21, 2014 at 8:55 AM, Thierry Reding > wrote: > > From: Thierry Reding > > > > Non-legacy devices may not always support mode-setting functionality, so > > create the primary minor conditionally. > > > > One setup where this happens is the Tegra K1, where the Tegra DRM driver > > exposes the display engine via standard KMS IOCTLs, and nouveau drives > > the Kepler-type GPU that has no display capabilities. > > > > Signed-off-by: Thierry Reding > > --- > > drivers/gpu/drm/drm_stub.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c > > index fd2f1758366d..839460b774c5 100644 > > --- a/drivers/gpu/drm/drm_stub.c > > +++ b/drivers/gpu/drm/drm_stub.c > > @@ -564,9 +564,11 @@ struct drm_device *drm_dev_alloc(struct drm_driver= *driver, > > goto err_minors; > > } > > > > - ret =3D drm_minor_alloc(dev, DRM_MINOR_PRIMARY); > > - if (ret) > > - goto err_minors; > > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > > + ret =3D drm_minor_alloc(dev, DRM_MINOR_PRIMARY); > > + if (ret) > > + goto err_minors; > > + } >=20 > There's a lot of code accessing dev->primary for debug messages (to > print stuff like "error on card0: bla"). I just want to make sure you > checked for all that. I tried renaming "->primary" to "->primary2" > locally just to find these and I doubt this is safe for most drivers. > I haven't looked for nouveau in particular, though. > Anyhow, the patch is the right thing to do. I've looked into this a little more to find out where the drm_device's =2Eprimary field is used, with the following results: 1) DRM core: - drm_bufs.c: Used in drm_find_matching_map() and drm_addmap_core(). This code is exclusively used by drivers that advertise the LEGACY feature as far as I can tell. So these would be candidates where not allocating the primary minor will cause problems. But I don't understanding of the legacy code paths well enough to propose a solution that could be applied here. - drm_fb_helper.c: This is only used by drivers advertising the MODESET feature, so shouldn't be an issue. - drm_stub.c: All accesses to the primary minor are protected by a check for the MODESET feature. - drm_sysfs.c: Used primarily for hotplug events, which from what I can tell are MODESET only. One oddity here is the __drm_class_suspend() function, which checks for: 46 if (drm_minor->type =3D=3D DRM_MINOR_PRIMARY && 47 drm_core_check_feature(drm_dev, DRIVER_LEGACY) && 48 drm_dev->driver->suspend) Which is somewhat confusing to me. drm_class_resume() performs the same check. 2) DRM drivers: - armada, ast, bochs, cirrus, exynos, i810, imx-drm, mga, mgag200, msm, nouveau, omapdrm, r128, rcar-du, savage, shmobile, sis, tdfx, tildc, udl, via and vmwgfx don't use the primary directly at all. - i915: - i915_dma.c and i915_irq.c protect all accesses to the primary minor with LEGACY feature checks - intel_display.c: uses it for SAREA code, but I think that's scheduled to be removed (I do remember some discussion about this on IRC not so long ago). Adding Daniel on Cc, maybe he knows more about this. - intel_ringbuffer.c: This seems to be SAREA related as well. - i915_sysfs.c: sysfs code is used by both legacy and non-legacy drivers, so this could be problematic. - qxl: Uses the primary minor for debugfs support, but the driver is unconditonally MODESET so it shouldn't be an issue. - radeon: Uses the primary minor for debugfs support but all paths that initialize debugfs support are conditionally MODESET. - tegra: Uses the primary minor for debugfs support, but the driver is unconditionally MODESET so it shouldn't be an issue. One pretty common pattern, even though it shouldn't be problematic, is how drivers use the primary minor for debugfs support. That means that if the primary minor weren't registered, there'd be no place for these drivers to put their debugfs files. In all the above cases, the driver uses debugfs only for things that are modesetting related, so I guess there isn't an immediate problem. Interestingly nouveau seems to side-step this issue entirely by simply setting up debugfs for every minor. For other drivers it seems like the solution wouldn't be as easy, since there are many more files in debugfs and they can come from subdevices as well. Thierry --ncSAzJYg3Aa9+CRW Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTDLh+AAoJEN0jrNd/PrOh6CcQALu0tkHUzXP+axA4EEaUYFF/ tUHG+DpNe+RMF8/M5K23g7jxBLeak7l2h5nTEImHkgHsFB9jpSXSgW1jgeH9EBBT hDKdJ8mcjLm2W3fyNUAKQl+teQjeJYTsg/niFUE47EKRRj6O1Y8qN0OBD5YCgIuY EDlm7pNyhjCpYV83yAAKCqOn7HQ7MAGfZHyavdN8xXoXXyEF/71axlY4MXSlfWPD x/fjsQ5m+yFhffEZprRVkgVyopnv1QmmPVfDogJQ15hPTEJ9C98TlCQGCQf0E2Iq wEMXNy1PYbLjdXftVSFI7QFJg6qXcfkbgNGkPE+F/XBtJNLoUBCj7D5TS3D8gRO4 96xE+mGgT2cnm/Wlqo8/PaUUZFCBseuixKHqPd3UnXjR89/fOFip6680bapuPisT Cw65YySJkYLNfrqy431i2Lw00IT3Hqdhq4FP1mHxjSFCf2hjUTTwnljzdrGZGAnk KFuYPdeFdxgFBLS1ok7ngPyEZaYzZ/Wcfb8NrN+uBeIudKi7GPJUONiFKTCzR793 //ft4eKiPoBlIP/vnc0w0Wz3EC870WwG0aa4poeKFNZ81cEh1YynGgADbzH7wtYG mp5D0qx83/0gjq+3kudWLgfNXVHl5fPyWtKG2ZXppZ9wQU6yIbQlvs4r/Dxj3XVO t/IsoyLcfii6Zy67ryXp =/F/5 -----END PGP SIGNATURE----- --ncSAzJYg3Aa9+CRW-- --===============1074205720== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1074205720==--