Hi Daniel Am 28.02.20 um 18:43 schrieb Daniel Vetter: > On Fri, Feb 28, 2020 at 12:46 PM Thomas Zimmermann wrote: >> >> Hi >> >> Am 28.02.20 um 09:40 schrieb Daniel Vetter: >>> On Fri, Feb 28, 2020 at 8:44 AM Thomas Zimmermann wrote: >>>> >>>> Hi Daniel >>>> >>>> Am 27.02.20 um 19:15 schrieb Daniel Vetter: >>>>> There's only two functions called from that: >>>>> drm_kms_helper_poll_fini() and udl_free_urb_list(). Both of these are >>>>> also called from the ubs_driver->disconnect hook, so entirely >>>>> pointless to do the same again in the ->release hook. >>>> >>>> The disconnect hook calls drm_kms_helper_poll_disable() instead if >>>> _fini(). They are the same, except that _disable() does not clear >>>> dev->mode_config.poll_enabled to false. Is this OK? >>> >>> oops, I overlooked that. But yeah for driver shutdown it's the same >>> really, we're not going to re-enable. _disable is meant for suspend so >>> youc an re-enable again on resume. >>> >>> I'll augment the commit message on the next round to clarify that. >> >> Well, we have a managed API. It could support >> drmm_kms_helper_poll_init(). :) > > You're ahead of the game here, but yes that's the plan. And a lot > more. Ideally I really want to get rid of both bus_driver->remove and > drm_driver->release callbacks for all drivers. > > Also, for polling you actually want devm_kms_poll_init, since polling > should be stopped at unplug/remove time. Not at drm_driver release > time :-) Quite honestly, if you're not adding devm_kms_poll_init() now, why even bother removing the _fini() call from release()? It hasn't been a problem so far and it won't become one. Doing a half-baked change now results in a potential WTF moment for other developers. Rather clean up release() when you add the managed devm_kms_poll_init() and have a nice, clear patch. Best regards Thomas > -Daniel > >> >> Best regards >> Thomas >> >>> -Daniel >>> >>> >>>> Best regards >>>> Thomas >>>> >>>>> >>>>> Furthermore by the time we clean up the drm_driver we really shouldn't >>>>> be touching hardware anymore, so stopping the poll worker and freeing >>>>> the urb allocations in ->disconnect is the right thing to do. >>>>> >>>>> Now disconnect still cleans things up before unregistering the driver, >>>>> but that's a different issue. >>>>> >>>>> Signed-off-by: Daniel Vetter >>>>> Cc: Dave Airlie >>>>> Cc: Sean Paul >>>>> Cc: Daniel Vetter >>>>> Cc: Thomas Zimmermann >>>>> Cc: Emil Velikov >>>>> Cc: Gerd Hoffmann >>>>> Cc: "Noralf Trønnes" >>>>> Cc: Sam Ravnborg >>>>> Cc: Thomas Gleixner >>>>> Cc: Alex Deucher >>>>> --- >>>>> drivers/gpu/drm/udl/udl_drv.c | 6 ------ >>>>> drivers/gpu/drm/udl/udl_drv.h | 1 - >>>>> drivers/gpu/drm/udl/udl_main.c | 10 ---------- >>>>> 3 files changed, 17 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >>>>> index b447fb053e78..7f140898df3e 100644 >>>>> --- a/drivers/gpu/drm/udl/udl_drv.c >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c >>>>> @@ -34,14 +34,8 @@ static int udl_usb_resume(struct usb_interface *interface) >>>>> >>>>> DEFINE_DRM_GEM_FOPS(udl_driver_fops); >>>>> >>>>> -static void udl_driver_release(struct drm_device *dev) >>>>> -{ >>>>> - udl_fini(dev); >>>>> -} >>>>> - >>>>> static struct drm_driver driver = { >>>>> .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, >>>>> - .release = udl_driver_release, >>>>> >>>>> /* gem hooks */ >>>>> .gem_create_object = udl_driver_gem_create_object, >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h >>>>> index 1de7eb1b6aac..2642f94a63fc 100644 >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h >>>>> @@ -76,7 +76,6 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len); >>>>> void udl_urb_completion(struct urb *urb); >>>>> >>>>> int udl_init(struct udl_device *udl); >>>>> -void udl_fini(struct drm_device *dev); >>>>> >>>>> int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, >>>>> const char *front, char **urb_buf_ptr, >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c >>>>> index 538718919916..f5d27f2a5654 100644 >>>>> --- a/drivers/gpu/drm/udl/udl_main.c >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c >>>>> @@ -351,13 +351,3 @@ int udl_drop_usb(struct drm_device *dev) >>>>> udl_free_urb_list(dev); >>>>> return 0; >>>>> } >>>>> - >>>>> -void udl_fini(struct drm_device *dev) >>>>> -{ >>>>> - struct udl_device *udl = to_udl(dev); >>>>> - >>>>> - drm_kms_helper_poll_fini(dev); >>>>> - >>>>> - if (udl->urbs.count) >>>>> - udl_free_urb_list(dev); >>>>> -} >>>>> >>>> >>>> -- >>>> 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 >>>> >>> >>> >> >> -- >> 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 >> > > -- 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