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@xxxxxxxxx> wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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:
pgpMs29JVJgo5.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
