| Message ID | 20200403135828.2542770-16-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | devm_drm_dev_alloc, no more drmm_add_final_kfree | expand |
Den 03.04.2020 15.57, skrev Daniel Vetter: > Also init the fbdev emulation before we register the device, that way > we can rely on the auto-cleanup and simplify the probe error code even > more. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > drivers/gpu/drm/udl/udl_drv.c | 36 +++++++++++------------------------ > 1 file changed, 11 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > index 1ce2d865c36d..4ba5149fdd57 100644 > --- a/drivers/gpu/drm/udl/udl_drv.c > +++ b/drivers/gpu/drm/udl/udl_drv.c > @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface) > struct udl_device *udl; > int r; > > - udl = kzalloc(sizeof(*udl), GFP_KERNEL); > - if (!udl) > - return ERR_PTR(-ENOMEM); > - > - r = drm_dev_init(&udl->drm, &driver, &interface->dev); > - if (r) { > - kfree(udl); > - return ERR_PTR(r); > - } > + udl = devm_drm_dev_alloc(&interface->dev, &driver, > + struct udl_device, drm); > + if (IS_ERR(udl)) > + return udl; > > udl->udev = udev; > udl->drm.dev_private = udl; > - drmm_add_final_kfree(&udl->drm, udl); > > r = udl_init(udl); > - if (r) { > - drm_dev_put(&udl->drm); > + if (r) > return ERR_PTR(r); > - } > > usb_set_intfdata(interface, udl); > + > return udl; > } > > @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface *interface, > if (IS_ERR(udl)) > return PTR_ERR(udl); > > + r = drm_fbdev_generic_setup(&udl->drm, 0); It doesn't feel right to have a client open the device before the DRM device itself is registered. I would prefer to keep it where it is but ignore any errors. A failing client shouldn't prevent the driver from probing. drm_fbdev_generic_setup() do print errors if it fails. So yeah, in hindsight I should have made drm_fbdev_generic_setup() return void. Noralf. > + if (r) > + return r; > + > r = drm_dev_register(&udl->drm, 0); > if (r) > - goto err_free; > + return r; > > DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index); > > - r = drm_fbdev_generic_setup(&udl->drm, 0); > - if (r) > - goto err_drm_dev_unregister; > - > return 0; > - > -err_drm_dev_unregister: > - drm_dev_unregister(&udl->drm); > -err_free: > - drm_dev_put(&udl->drm); > - return r; > } > > static void udl_usb_disconnect(struct usb_interface *interface) > @@ -117,7 +104,6 @@ static void udl_usb_disconnect(struct usb_interface *interface) > drm_kms_helper_poll_fini(dev); > udl_drop_usb(dev); > drm_dev_unplug(dev); > - drm_dev_put(dev); > } > > /* >
On Sun, Apr 5, 2020 at 12:18 PM Noralf Trønnes <noralf@tronnes.org> wrote: > > > > Den 03.04.2020 15.57, skrev Daniel Vetter: > > Also init the fbdev emulation before we register the device, that way > > we can rely on the auto-cleanup and simplify the probe error code even > > more. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > --- > > drivers/gpu/drm/udl/udl_drv.c | 36 +++++++++++------------------------ > > 1 file changed, 11 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > > index 1ce2d865c36d..4ba5149fdd57 100644 > > --- a/drivers/gpu/drm/udl/udl_drv.c > > +++ b/drivers/gpu/drm/udl/udl_drv.c > > @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface) > > struct udl_device *udl; > > int r; > > > > - udl = kzalloc(sizeof(*udl), GFP_KERNEL); > > - if (!udl) > > - return ERR_PTR(-ENOMEM); > > - > > - r = drm_dev_init(&udl->drm, &driver, &interface->dev); > > - if (r) { > > - kfree(udl); > > - return ERR_PTR(r); > > - } > > + udl = devm_drm_dev_alloc(&interface->dev, &driver, > > + struct udl_device, drm); > > + if (IS_ERR(udl)) > > + return udl; > > > > udl->udev = udev; > > udl->drm.dev_private = udl; > > - drmm_add_final_kfree(&udl->drm, udl); > > > > r = udl_init(udl); > > - if (r) { > > - drm_dev_put(&udl->drm); > > + if (r) > > return ERR_PTR(r); > > - } > > > > usb_set_intfdata(interface, udl); > > + > > return udl; > > } > > > > @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface *interface, > > if (IS_ERR(udl)) > > return PTR_ERR(udl); > > > > + r = drm_fbdev_generic_setup(&udl->drm, 0); > > It doesn't feel right to have a client open the device before the DRM > device itself is registered. I would prefer to keep it where it is but > ignore any errors. A failing client shouldn't prevent the driver from > probing. drm_fbdev_generic_setup() do print errors if it fails. So yeah, > in hindsight I should have made drm_fbdev_generic_setup() return void. Hm, we have all kinds of usage right now, some check for errors, some dont, some do this before drm_dev_register, some after. If your recommendation is to not check for errors then I'm happy to implement that, but we're a bit inconsistent. Maybe we should do a patch that at least always returns 0 no matter what, plus document that the return value shouldn't be checked? -Daniel > > Noralf. > > > + if (r) > > + return r; > > + > > r = drm_dev_register(&udl->drm, 0); > > if (r) > > - goto err_free; > > + return r; > > > > DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index); > > > > - r = drm_fbdev_generic_setup(&udl->drm, 0); > > - if (r) > > - goto err_drm_dev_unregister; > > - > > return 0; > > - > > -err_drm_dev_unregister: > > - drm_dev_unregister(&udl->drm); > > -err_free: > > - drm_dev_put(&udl->drm); > > - return r; > > } > > > > static void udl_usb_disconnect(struct usb_interface *interface) > > @@ -117,7 +104,6 @@ static void udl_usb_disconnect(struct usb_interface *interface) > > drm_kms_helper_poll_fini(dev); > > udl_drop_usb(dev); > > drm_dev_unplug(dev); > > - drm_dev_put(dev); > > } > > > > /* > >
Den 05.04.2020 15.47, skrev Daniel Vetter: > On Sun, Apr 5, 2020 at 12:18 PM Noralf Trønnes <noralf@tronnes.org> wrote: >> >> >> >> Den 03.04.2020 15.57, skrev Daniel Vetter: >>> Also init the fbdev emulation before we register the device, that way >>> we can rely on the auto-cleanup and simplify the probe error code even >>> more. >>> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Sean Paul <sean@poorly.run> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Emil Velikov <emil.l.velikov@gmail.com> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> --- >>> drivers/gpu/drm/udl/udl_drv.c | 36 +++++++++++------------------------ >>> 1 file changed, 11 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >>> index 1ce2d865c36d..4ba5149fdd57 100644 >>> --- a/drivers/gpu/drm/udl/udl_drv.c >>> +++ b/drivers/gpu/drm/udl/udl_drv.c >>> @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface) >>> struct udl_device *udl; >>> int r; >>> >>> - udl = kzalloc(sizeof(*udl), GFP_KERNEL); >>> - if (!udl) >>> - return ERR_PTR(-ENOMEM); >>> - >>> - r = drm_dev_init(&udl->drm, &driver, &interface->dev); >>> - if (r) { >>> - kfree(udl); >>> - return ERR_PTR(r); >>> - } >>> + udl = devm_drm_dev_alloc(&interface->dev, &driver, >>> + struct udl_device, drm); >>> + if (IS_ERR(udl)) >>> + return udl; >>> >>> udl->udev = udev; >>> udl->drm.dev_private = udl; >>> - drmm_add_final_kfree(&udl->drm, udl); >>> >>> r = udl_init(udl); >>> - if (r) { >>> - drm_dev_put(&udl->drm); >>> + if (r) >>> return ERR_PTR(r); >>> - } >>> >>> usb_set_intfdata(interface, udl); >>> + >>> return udl; >>> } >>> >>> @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface *interface, >>> if (IS_ERR(udl)) >>> return PTR_ERR(udl); >>> >>> + r = drm_fbdev_generic_setup(&udl->drm, 0); >> >> It doesn't feel right to have a client open the device before the DRM >> device itself is registered. I would prefer to keep it where it is but >> ignore any errors. A failing client shouldn't prevent the driver from >> probing. drm_fbdev_generic_setup() do print errors if it fails. So yeah, >> in hindsight I should have made drm_fbdev_generic_setup() return void. > > Hm, we have all kinds of usage right now, some check for errors, some > dont, some do this before drm_dev_register, some after. If your > recommendation is to not check for errors then I'm happy to implement > that, but we're a bit inconsistent. Maybe we should do a patch that at > least always returns 0 no matter what, plus document that the return > value shouldn't be checked? Yeah, always returning zero and documenting it would be a good start. I counted 41 drivers using generic fbdev now, didn't know it was that much used. Only 11 drivers are hand rolling their own: armada gma500 amd omapdrm nouveau i915 msm tegra exynos radeon rockchip Noralf.
Am 05.04.20 um 12:18 schrieb Noralf Trønnes: > > > Den 03.04.2020 15.57, skrev Daniel Vetter: >> Also init the fbdev emulation before we register the device, that way >> we can rely on the auto-cleanup and simplify the probe error code even >> more. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Dave Airlie <airlied@redhat.com> >> Cc: Sean Paul <sean@poorly.run> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Emil Velikov <emil.l.velikov@gmail.com> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> --- >> drivers/gpu/drm/udl/udl_drv.c | 36 +++++++++++------------------------ >> 1 file changed, 11 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >> index 1ce2d865c36d..4ba5149fdd57 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.c >> +++ b/drivers/gpu/drm/udl/udl_drv.c >> @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface) >> struct udl_device *udl; >> int r; >> >> - udl = kzalloc(sizeof(*udl), GFP_KERNEL); >> - if (!udl) >> - return ERR_PTR(-ENOMEM); >> - >> - r = drm_dev_init(&udl->drm, &driver, &interface->dev); >> - if (r) { >> - kfree(udl); >> - return ERR_PTR(r); >> - } >> + udl = devm_drm_dev_alloc(&interface->dev, &driver, >> + struct udl_device, drm); >> + if (IS_ERR(udl)) >> + return udl; >> >> udl->udev = udev; >> udl->drm.dev_private = udl; >> - drmm_add_final_kfree(&udl->drm, udl); >> >> r = udl_init(udl); >> - if (r) { >> - drm_dev_put(&udl->drm); >> + if (r) >> return ERR_PTR(r); >> - } >> >> usb_set_intfdata(interface, udl); >> + >> return udl; >> } >> >> @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface *interface, >> if (IS_ERR(udl)) >> return PTR_ERR(udl); >> >> + r = drm_fbdev_generic_setup(&udl->drm, 0); > > It doesn't feel right to have a client open the device before the DRM > device itself is registered. I would prefer to keep it where it is but Agreed. IMHO we should also go through drivers and make the fbdev setup the final step everywhere. Best regards Thomas > ignore any errors. A failing client shouldn't prevent the driver from > probing. drm_fbdev_generic_setup() do print errors if it fails. So yeah, > in hindsight I should have made drm_fbdev_generic_setup() return void. > > Noralf. > >> + if (r) >> + return r; >> + >> r = drm_dev_register(&udl->drm, 0); >> if (r) >> - goto err_free; >> + return r; >> >> DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index); >> >> - r = drm_fbdev_generic_setup(&udl->drm, 0); >> - if (r) >> - goto err_drm_dev_unregister; >> - >> return 0; >> - >> -err_drm_dev_unregister: >> - drm_dev_unregister(&udl->drm); >> -err_free: >> - drm_dev_put(&udl->drm); >> - return r; >> } >> >> static void udl_usb_disconnect(struct usb_interface *interface) >> @@ -117,7 +104,6 @@ static void udl_usb_disconnect(struct usb_interface *interface) >> drm_kms_helper_poll_fini(dev); >> udl_drop_usb(dev); >> drm_dev_unplug(dev); >> - drm_dev_put(dev); >> } >> >> /* >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 1ce2d865c36d..4ba5149fdd57 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface) struct udl_device *udl; int r; - udl = kzalloc(sizeof(*udl), GFP_KERNEL); - if (!udl) - return ERR_PTR(-ENOMEM); - - r = drm_dev_init(&udl->drm, &driver, &interface->dev); - if (r) { - kfree(udl); - return ERR_PTR(r); - } + udl = devm_drm_dev_alloc(&interface->dev, &driver, + struct udl_device, drm); + if (IS_ERR(udl)) + return udl; udl->udev = udev; udl->drm.dev_private = udl; - drmm_add_final_kfree(&udl->drm, udl); r = udl_init(udl); - if (r) { - drm_dev_put(&udl->drm); + if (r) return ERR_PTR(r); - } usb_set_intfdata(interface, udl); + return udl; } @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface *interface, if (IS_ERR(udl)) return PTR_ERR(udl); + r = drm_fbdev_generic_setup(&udl->drm, 0); + if (r) + return r; + r = drm_dev_register(&udl->drm, 0); if (r) - goto err_free; + return r; DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index); - r = drm_fbdev_generic_setup(&udl->drm, 0); - if (r) - goto err_drm_dev_unregister; - return 0; - -err_drm_dev_unregister: - drm_dev_unregister(&udl->drm); -err_free: - drm_dev_put(&udl->drm); - return r; } static void udl_usb_disconnect(struct usb_interface *interface) @@ -117,7 +104,6 @@ static void udl_usb_disconnect(struct usb_interface *interface) drm_kms_helper_poll_fini(dev); udl_drop_usb(dev); drm_dev_unplug(dev); - drm_dev_put(dev); } /*
Also init the fbdev emulation before we register the device, that way we can rely on the auto-cleanup and simplify the probe error code even more. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Dave Airlie <airlied@redhat.com> Cc: Sean Paul <sean@poorly.run> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Emil Velikov <emil.l.velikov@gmail.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Gleixner <tglx@linutronix.de> --- drivers/gpu/drm/udl/udl_drv.c | 36 +++++++++++------------------------ 1 file changed, 11 insertions(+), 25 deletions(-)