| Message ID | 1391004120-687-12-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 29, 2014 at 03:01:58PM +0100, David Herrmann wrote: > Whenever we access minor->device, we are in a minor->kdev->...->fops > callback so the minor->kdev pointer *must* be valid. Thus, simply use > minor->kdev->devt instead of minor->device and remove the redundant field. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> I think this is simply compat cruft from the days when the drm core was still shared with the *bsds. With the one patch I've commented on all patches up to this one are Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> As discussed on irc I think we don't want to have stable minor ids really, userspace simply needs to inquire udev to get at the right render/control/legacy node it wants. There's also ongoing discussions about how that probing should look like on e.g. ARM. So at least for now Nack on those two from my side. Wrt longer-term plans I think the minors (and also all the sysfs stuff really) also need to grab references on the drm_device, so that unplugging doesn't race with userspace. But I guess that this might lead to reference loops for driver unloading, so maybe we need to postpone that a bit more. But this is definitely a big step into the right direction. Cheers, Daniel
On Wed, Feb 12, 2014 at 02:36:24PM +0100, Daniel Vetter wrote: > On Wed, Jan 29, 2014 at 03:01:58PM +0100, David Herrmann wrote: > > Whenever we access minor->device, we are in a minor->kdev->...->fops > > callback so the minor->kdev pointer *must* be valid. Thus, simply use > > minor->kdev->devt instead of minor->device and remove the redundant field. > > > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > I think this is simply compat cruft from the days when the drm core was > still shared with the *bsds. With the one patch I've commented on all > patches up to this one are > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > As discussed on irc I think we don't want to have stable minor ids really, > userspace simply needs to inquire udev to get at the right > render/control/legacy node it wants. Does that mean we should go all the way and don't keep the +64 (for control) and +128 (for render nodes) offsets either? Should it be possible to have a /dev/dri directory that looks somewhat like this: /dev/dri/card0 (GPU#0, legacy) /dev/dri/card1 (GPU#1, legacy) /dev/dri/render0 (GPU#1, render) ? Thierry
Hi On Fri, Feb 21, 2014 at 8:30 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Feb 12, 2014 at 02:36:24PM +0100, Daniel Vetter wrote: >> On Wed, Jan 29, 2014 at 03:01:58PM +0100, David Herrmann wrote: >> > Whenever we access minor->device, we are in a minor->kdev->...->fops >> > callback so the minor->kdev pointer *must* be valid. Thus, simply use >> > minor->kdev->devt instead of minor->device and remove the redundant field. >> > >> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> >> I think this is simply compat cruft from the days when the drm core was >> still shared with the *bsds. With the one patch I've commented on all >> patches up to this one are >> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> As discussed on irc I think we don't want to have stable minor ids really, >> userspace simply needs to inquire udev to get at the right >> render/control/legacy node it wants. > > Does that mean we should go all the way and don't keep the +64 (for > control) and +128 (for render nodes) offsets either? Should it be > possible to have a /dev/dri directory that looks somewhat like this: > > /dev/dri/card0 (GPU#0, legacy) > /dev/dri/card1 (GPU#1, legacy) > /dev/dri/render0 (GPU#1, render) That might break backwards compat, but may be worth it. However, we *have* to keep the +64 / +128 offsets for minor numbers. There's already user-space using that for dev-type testing (which is fine!). Thanks David
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 345be03..ec651be 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -344,7 +344,7 @@ long drm_ioctl(struct file *filp, DRM_DEBUG("pid=%d, dev=0x%lx, auth=%d, %s\n", task_pid_nr(current), - (long)old_encode_dev(file_priv->minor->device), + (long)old_encode_dev(file_priv->minor->kdev->devt), file_priv->authenticated, ioctl->name); /* Do not trust userspace, use our own definition */ @@ -402,7 +402,7 @@ long drm_ioctl(struct file *filp, if (!ioctl) DRM_DEBUG("invalid ioctl: pid=%d, dev=0x%lx, auth=%d, cmd=0x%02x, nr=0x%02x\n", task_pid_nr(current), - (long)old_encode_dev(file_priv->minor->device), + (long)old_encode_dev(file_priv->minor->kdev->devt), file_priv->authenticated, cmd, nr); if (kdata != stack_kdata) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 7947819..4ce5318 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -471,7 +471,7 @@ int drm_release(struct inode *inode, struct file *filp) DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n", task_pid_nr(current), - (long)old_encode_dev(file_priv->minor->device), + (long)old_encode_dev(file_priv->minor->kdev->devt), dev->open_count); /* Release any auth tokens that might point to this file_priv, diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index d564a5d..fa9542e 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -318,7 +318,6 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) if (minor_id < 0) return minor_id; - new_minor->device = MKDEV(DRM_MAJOR, minor_id); new_minor->index = minor_id; idr_replace(&drm_minors_idr, new_minor, minor_id); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3c02cad..114d46f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1040,7 +1040,6 @@ struct drm_info_node { struct drm_minor { int index; /**< Minor device number */ int type; /**< Control or render */ - dev_t device; /**< Device number for mknod */ struct device *kdev; /**< Linux device */ struct drm_device *dev;
Whenever we access minor->device, we are in a minor->kdev->...->fops callback so the minor->kdev pointer *must* be valid. Thus, simply use minor->kdev->devt instead of minor->device and remove the redundant field. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_drv.c | 4 ++-- drivers/gpu/drm/drm_fops.c | 2 +- drivers/gpu/drm/drm_stub.c | 1 - include/drm/drmP.h | 1 - 4 files changed, 3 insertions(+), 5 deletions(-)