| Message ID | 20200227181522.2711142-51-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm managed resources, v3 | expand |
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? 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 <daniel.vetter@intel.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Alex Deucher <alexander.deucher@amd.com> > --- > 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); > -} >
On Fri, Feb 28, 2020 at 8:44 AM Thomas Zimmermann <tzimmermann@suse.de> 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. -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 <daniel.vetter@intel.com> > > Cc: Dave Airlie <airlied@redhat.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Alex Deucher <alexander.deucher@amd.com> > > --- > > 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 >
Hi Am 28.02.20 um 09:40 schrieb Daniel Vetter: > On Fri, Feb 28, 2020 at 8:44 AM Thomas Zimmermann <tzimmermann@suse.de> 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(). :) 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 <daniel.vetter@intel.com> >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Sean Paul <sean@poorly.run> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Emil Velikov <emil.l.velikov@gmail.com> >>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>> Cc: "Noralf Trønnes" <noralf@tronnes.org> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> --- >>> 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 >> > >
On Fri, Feb 28, 2020 at 12:46 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 28.02.20 um 09:40 schrieb Daniel Vetter: > > On Fri, Feb 28, 2020 at 8:44 AM Thomas Zimmermann <tzimmermann@suse.de> 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 :-) -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 <daniel.vetter@intel.com> > >>> Cc: Dave Airlie <airlied@redhat.com> > >>> Cc: Sean Paul <sean@poorly.run> > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>> Cc: Emil Velikov <emil.l.velikov@gmail.com> > >>> Cc: Gerd Hoffmann <kraxel@redhat.com> > >>> Cc: "Noralf Trønnes" <noralf@tronnes.org> > >>> Cc: Sam Ravnborg <sam@ravnborg.org> > >>> Cc: Thomas Gleixner <tglx@linutronix.de> > >>> Cc: Alex Deucher <alexander.deucher@amd.com> > >>> --- > >>> 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 >
Hi Daniel Am 28.02.20 um 18:43 schrieb Daniel Vetter: > On Fri, Feb 28, 2020 at 12:46 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 28.02.20 um 09:40 schrieb Daniel Vetter: >>> On Fri, Feb 28, 2020 at 8:44 AM Thomas Zimmermann <tzimmermann@suse.de> 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 <daniel.vetter@intel.com> >>>>> Cc: Dave Airlie <airlied@redhat.com> >>>>> Cc: Sean Paul <sean@poorly.run> >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com> >>>>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>>>> Cc: "Noralf Trønnes" <noralf@tronnes.org> >>>>> Cc: Sam Ravnborg <sam@ravnborg.org> >>>>> Cc: Thomas Gleixner <tglx@linutronix.de> >>>>> Cc: Alex Deucher <alexander.deucher@amd.com> >>>>> --- >>>>> 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 >> > >
On Mon, Mar 2, 2020 at 9:14 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi Daniel > > Am 28.02.20 um 18:43 schrieb Daniel Vetter: > > On Fri, Feb 28, 2020 at 12:46 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi > >> > >> Am 28.02.20 um 09:40 schrieb Daniel Vetter: > >>> On Fri, Feb 28, 2020 at 8:44 AM Thomas Zimmermann <tzimmermann@suse.de> 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. So the thing is, you need to stop your poll worker in ->unplug, not ->release. That's the part I'm fixing here (plus stopping it twice is overkill). I'm not seeing any connection to making the ->unplug part here automatic, that was just a future idea. But this patch series here is all about ->release stuff. I guess I can do a respin and change the one in ->unplug from _disable to _fini, which would be clearer. But not doing this patch here just because we want to clean things up even more doesn't make sense to me. -Daniel > 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 <daniel.vetter@intel.com> > >>>>> Cc: Dave Airlie <airlied@redhat.com> > >>>>> Cc: Sean Paul <sean@poorly.run> > >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com> > >>>>> Cc: Gerd Hoffmann <kraxel@redhat.com> > >>>>> Cc: "Noralf Trønnes" <noralf@tronnes.org> > >>>>> Cc: Sam Ravnborg <sam@ravnborg.org> > >>>>> Cc: Thomas Gleixner <tglx@linutronix.de> > >>>>> Cc: Alex Deucher <alexander.deucher@amd.com> > >>>>> --- > >>>>> 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 >
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); -}
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. 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 <daniel.vetter@intel.com> Cc: Dave Airlie <airlied@redhat.com> Cc: Sean Paul <sean@poorly.run> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Emil Velikov <emil.l.velikov@gmail.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: "Noralf Trønnes" <noralf@tronnes.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Alex Deucher <alexander.deucher@amd.com> --- 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(-)