Re: [RFC 05/12] drm: Create primary minor only if mode-setting is supported

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4905 bytes --]

On Mon, Feb 24, 2014 at 11:39:33AM +0100, David Herrmann wrote:
> Hi
> 
> On Fri, Feb 21, 2014 at 8:55 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > 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 <treding@nvidia.com>
> > ---
> >  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 = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> > -       if (ret)
> > -               goto err_minors;
> > +       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > +               ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> > +               if (ret)
> > +                       goto err_minors;
> > +       }
> 
> 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
.primary 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 == 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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel