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