Re: [Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
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 >> Cc: Dave Airlie >> Cc: Sean Paul >> Cc: Thomas Zimmermann >> Cc: Daniel Vetter >> Cc: Emil Velikov >> Cc: Sam Ravnborg >> Cc: Thomas Gleixner >> --- >> 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(>drm, , >dev); >> -if (r) { >> -kfree(udl); >> -return ERR_PTR(r); >> -} >> +udl = devm_drm_dev_alloc(>dev, , >> + struct udl_device, drm); >> +if (IS_ERR(udl)) >> +return udl; >> >> udl->udev = udev; >> udl->drm.dev_private = udl; >> -drmm_add_final_kfree(>drm, udl); >> >> r = udl_init(udl); >> -if (r) { >> -drm_dev_put(>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(>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(>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(>drm, 0); >> -if (r) >> -goto err_drm_dev_unregister; >> - >> return 0; >> - >> -err_drm_dev_unregister: >> -drm_dev_unregister(>drm); >> -err_free: >> -drm_dev_put(>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-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
Den 05.04.2020 15.47, skrev Daniel Vetter: > On Sun, Apr 5, 2020 at 12:18 PM Noralf Trønnes 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 >>> Cc: Dave Airlie >>> Cc: Sean Paul >>> Cc: Thomas Zimmermann >>> Cc: Daniel Vetter >>> Cc: Emil Velikov >>> Cc: Sam Ravnborg >>> Cc: Thomas Gleixner >>> --- >>> 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(>drm, , >dev); >>> - if (r) { >>> - kfree(udl); >>> - return ERR_PTR(r); >>> - } >>> + udl = devm_drm_dev_alloc(>dev, , >>> + struct udl_device, drm); >>> + if (IS_ERR(udl)) >>> + return udl; >>> >>> udl->udev = udev; >>> udl->drm.dev_private = udl; >>> - drmm_add_final_kfree(>drm, udl); >>> >>> r = udl_init(udl); >>> - if (r) { >>> - drm_dev_put(>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(>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. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
On Sun, Apr 5, 2020 at 12:18 PM Noralf Trønnes 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 > > Cc: Dave Airlie > > Cc: Sean Paul > > Cc: Thomas Zimmermann > > Cc: Daniel Vetter > > Cc: Emil Velikov > > Cc: Sam Ravnborg > > Cc: Thomas Gleixner > > --- > > 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(>drm, , >dev); > > - if (r) { > > - kfree(udl); > > - return ERR_PTR(r); > > - } > > + udl = devm_drm_dev_alloc(>dev, , > > + struct udl_device, drm); > > + if (IS_ERR(udl)) > > + return udl; > > > > udl->udev = udev; > > udl->drm.dev_private = udl; > > - drmm_add_final_kfree(>drm, udl); > > > > r = udl_init(udl); > > - if (r) { > > - drm_dev_put(>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(>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(>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(>drm, 0); > > - if (r) > > - goto err_drm_dev_unregister; > > - > > return 0; > > - > > -err_drm_dev_unregister: > > - drm_dev_unregister(>drm); > > -err_free: > > - drm_dev_put(>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); > > } > > > > /* > > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
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 > Cc: Dave Airlie > Cc: Sean Paul > Cc: Thomas Zimmermann > Cc: Daniel Vetter > Cc: Emil Velikov > Cc: Sam Ravnborg > Cc: Thomas Gleixner > --- > 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(>drm, , >dev); > - if (r) { > - kfree(udl); > - return ERR_PTR(r); > - } > + udl = devm_drm_dev_alloc(>dev, , > + struct udl_device, drm); > + if (IS_ERR(udl)) > + return udl; > > udl->udev = udev; > udl->drm.dev_private = udl; > - drmm_add_final_kfree(>drm, udl); > > r = udl_init(udl); > - if (r) { > - drm_dev_put(>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(>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(>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(>drm, 0); > - if (r) > - goto err_drm_dev_unregister; > - > return 0; > - > -err_drm_dev_unregister: > - drm_dev_unregister(>drm); > -err_free: > - drm_dev_put(>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); > } > > /* > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
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 Cc: Dave Airlie Cc: Sean Paul Cc: Thomas Zimmermann Cc: Daniel Vetter Cc: Emil Velikov Cc: Sam Ravnborg Cc: Thomas Gleixner --- 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(>drm, , >dev); - if (r) { - kfree(udl); - return ERR_PTR(r); - } + udl = devm_drm_dev_alloc(>dev, , +struct udl_device, drm); + if (IS_ERR(udl)) + return udl; udl->udev = udev; udl->drm.dev_private = udl; - drmm_add_final_kfree(>drm, udl); r = udl_init(udl); - if (r) { - drm_dev_put(>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(>drm, 0); + if (r) + return r; + r = drm_dev_register(>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(>drm, 0); - if (r) - goto err_drm_dev_unregister; - return 0; - -err_drm_dev_unregister: - drm_dev_unregister(>drm); -err_free: - drm_dev_put(>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); } /* -- 2.25.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx