Re: [Intel-gfx] [PATCH 50/51] drm/udl: drop drm_driver.release hook
Hi
Am 02.03.20 um 23:26 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.
>
> 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.
>
> v2: Use _fini, not _disable in unplug, motivated by discussions with
> Thomas. _disable/_enable are for suspend/resume.
>
> Signed-off-by: Daniel Vetter
Acked-by: Thomas Zimmermann
> 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 | 8 +---
> drivers/gpu/drm/udl/udl_drv.h | 1 -
> drivers/gpu/drm/udl/udl_main.c | 10 --
> 3 files changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index b447fb053e78..1ce2d865c36d 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,
> @@ -120,7 +114,7 @@ static void udl_usb_disconnect(struct usb_interface
> *interface)
> {
> struct drm_device *dev = usb_get_intfdata(interface);
>
> - drm_kms_helper_poll_disable(dev);
> + drm_kms_helper_poll_fini(dev);
> udl_drop_usb(dev);
> drm_dev_unplug(dev);
> drm_dev_put(dev);
> 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
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 50/51] drm/udl: drop drm_driver.release hook
On Mon, Mar 2, 2020 at 9:14 AM Thomas Zimmermann wrote:
>
> 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.
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
> > 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
Re: [Intel-gfx] [PATCH 50/51] drm/udl: drop drm_driver.release hook
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: Feli
Re: [Intel-gfx] [PATCH 50/51] drm/udl: drop drm_driver.release hook
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 :-)
-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
>
--
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/int
Re: [Intel-gfx] [PATCH 50/51] drm/udl: drop drm_driver.release hook
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(). :)
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
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 50/51] drm/udl: drop drm_driver.release hook
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.
-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
>
--
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 50/51] drm/udl: drop drm_driver.release hook
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
> 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
signature.asc
Description: OpenPGP digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
