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