Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Thu, Feb 07, 2019 at 04:07:33PM -0500, Sean Paul wrote:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> > The only thing now that makes drm_dev_unplug() special is that it sets
> > drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> > can remove drm_dev_unplug().
> >
> > Signed-off-by: Noralf Trønnes
> > ---
> >
> > Maybe s/unplugged/unregistered/ ?
> >
> > I looked at drm_device->registered, but using that would mean that
> > drm_dev_is_unplugged() would return before drm_device is registered.
> > And given that its current purpose is to prevent race against connector
> > registration, I stayed away from it.
>
> Makes sense. I think having unregistered along with registered might cause
> some
> confusion unless it's really clearly documented. Perhaps
> s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
> meaning...
>
> I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
> better off as a follow-on series where everyone can pile on opinions. So,
>
> Reviewed-by: Sean Paul
Looks like I was dropped from Cc, so I didn't see the replies after Oleksandr's
review. So go ahead and disregard this :-)
Sean
>
>
> >
> > Noralf.
> >
> >
> > drivers/gpu/drm/drm_drv.c | 27 +++
> > include/drm/drm_drv.h | 10 --
> > 2 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 05bbc2b622fc..e0941200edc6 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> > */
> > void drm_dev_unplug(struct drm_device *dev)
> > {
> > - /*
> > -* After synchronizing any critical read section is guaranteed to see
> > -* the new value of ->unplugged, and any critical section which might
> > -* still have seen the old value of ->unplugged is guaranteed to have
> > -* finished.
> > -*/
> > - dev->unplugged = true;
> > - synchronize_srcu(_unplug_srcu);
> > -
> > drm_dev_unregister(dev);
> > drm_dev_put(dev);
> > }
> > @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> > * drm_dev_register() but does not deallocate the device. The caller must
> > call
> > * drm_dev_put() to drop their final reference.
> > *
> > - * A special form of unregistering for hotpluggable devices is
> > drm_dev_unplug(),
> > - * which can be called while there are still open users of @dev.
> > + * This function can be called while there are still open users of @dev as
> > long
> > + * as the driver protects its device resources using drm_dev_enter() and
> > + * drm_dev_exit().
> > *
> > * This should be called first in the device teardown code to make sure
> > - * userspace can't access the device instance any more.
> > + * userspace can't access the device instance any more. Drivers that
> > support
> > + * device unplug will probably want to call drm_atomic_helper_shutdown()
> > first
> > + * in order to disable the hardware on regular driver module unload.
> > */
> > void drm_dev_unregister(struct drm_device *dev)
> > {
> > @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> > if (drm_core_check_feature(dev, DRIVER_LEGACY))
> > drm_lastclose(dev);
> >
> > + /*
> > +* After synchronizing any critical read section is guaranteed to see
> > +* the new value of ->unplugged, and any critical section which might
> > +* still have seen the old value of ->unplugged is guaranteed to have
> > +* finished.
> > +*/
> > + dev->unplugged = true;
> > + synchronize_srcu(_unplug_srcu);
> > +
> > dev->registered = false;
> >
> > drm_client_dev_unregister(dev);
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index ca46a45a9cce..c50696c82a42 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> > * drm_dev_is_unplugged - is a DRM device unplugged
> > * @dev: DRM device
> > *
> > - * This function can be called to check whether a hotpluggable is
> > unplugged.
> > - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> > - * unplugged, these two functions guarantee that any store before calling
> > - * drm_dev_unplug() is visible to callers of this function after it
> > completes
> > + * This function can be called to check whether @dev is unregistered. This
> > can
> > + * be used to detect that the underlying parent device is gone.
> > *
> > - * WARNING: This function fundamentally races against drm_dev_unplug(). It
> > is
> > - * recommended that drivers instead use the underlying drm_dev_enter() and
> > + * WARNING: This function fundamentally races against
> > drm_dev_unregister(). It
> > + * is recommended that drivers instead use the underlying drm_dev_enter()
> > and
> > * drm_dev_exit() function
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes
> ---
>
> Maybe s/unplugged/unregistered/ ?
>
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.
Makes sense. I think having unregistered along with registered might cause some
confusion unless it's really clearly documented. Perhaps
s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
meaning...
I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
better off as a follow-on series where everyone can pile on opinions. So,
Reviewed-by: Sean Paul
>
> Noralf.
>
>
> drivers/gpu/drm/drm_drv.c | 27 +++
> include/drm/drm_drv.h | 10 --
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> */
> void drm_dev_unplug(struct drm_device *dev)
> {
> - /*
> - * After synchronizing any critical read section is guaranteed to see
> - * the new value of ->unplugged, and any critical section which might
> - * still have seen the old value of ->unplugged is guaranteed to have
> - * finished.
> - */
> - dev->unplugged = true;
> - synchronize_srcu(_unplug_srcu);
> -
> drm_dev_unregister(dev);
> drm_dev_put(dev);
> }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> * drm_dev_register() but does not deallocate the device. The caller must
> call
> * drm_dev_put() to drop their final reference.
> *
> - * A special form of unregistering for hotpluggable devices is
> drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as
> long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
> *
> * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown()
> first
> + * in order to disable the hardware on regular driver module unload.
> */
> void drm_dev_unregister(struct drm_device *dev)
> {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> if (drm_core_check_feature(dev, DRIVER_LEGACY))
> drm_lastclose(dev);
>
> + /*
> + * After synchronizing any critical read section is guaranteed to see
> + * the new value of ->unplugged, and any critical section which might
> + * still have seen the old value of ->unplugged is guaranteed to have
> + * finished.
> + */
> + dev->unplugged = true;
> + synchronize_srcu(_unplug_srcu);
> +
> dev->registered = false;
>
> drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> * drm_dev_is_unplugged - is a DRM device unplugged
> * @dev: DRM device
> *
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This
> can
> + * be used to detect that the underlying parent device is gone.
> *
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the underlying drm_dev_enter() and
> + * WARNING: This function fundamentally races against drm_dev_unregister().
> It
> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> * drm_dev_exit() function pairs.
> */
> static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> --
> 2.20.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Wed, Feb 06, 2019 at 05:46:51PM +0100, Noralf Trønnes wrote:
>
>
> Den 06.02.2019 16.26, skrev Daniel Vetter:
> > On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 05.02.2019 17.31, skrev Daniel Vetter:
> >>> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
>
>
> Den 05.02.2019 10.11, skrev Daniel Vetter:
> > On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 04.02.2019 16.41, skrev Daniel Vetter:
> >>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it
> sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so
> that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes
> ---
> >>
> >> [...]
> >>
> drivers/gpu/drm/drm_drv.c | 27 +++
> include/drm/drm_drv.h | 10 --
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> */
> void drm_dev_unplug(struct drm_device *dev)
> {
> -/*
> - * After synchronizing any critical read section is guaranteed
> to see
> - * the new value of ->unplugged, and any critical section which
> might
> - * still have seen the old value of ->unplugged is guaranteed
> to have
> - * finished.
> - */
> -dev->unplugged = true;
> -synchronize_srcu(_unplug_srcu);
> -
> drm_dev_unregister(dev);
> drm_dev_put(dev);
> }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> * drm_dev_register() but does not deallocate the device. The
> caller must call
> * drm_dev_put() to drop their final reference.
> *
> - * A special form of unregistering for hotpluggable devices is
> drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of
> @dev as long
> + * as the driver protects its device resources using
> drm_dev_enter() and
> + * drm_dev_exit().
> *
> * This should be called first in the device teardown code to make
> sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers
> that support
> + * device unplug will probably want to call
> drm_atomic_helper_shutdown() first
> >>>
> >>> Read once more with a bit more coffee, spotted this:
> >>>
> >>> s/first/afterwards/ - shutting down the hw before we've taken it away
> >>> from
> >>> userspace is kinda the wrong way round. It should be the inverse of
> >>> driver
> >>> load, which is 1) allocate structures 2) prep hw 3) register driver
> >>> with
> >>> the world (simplified ofc).
> >>>
> >>
> >> The problem is that drm_dev_unregister() sets the device as unplugged
> >> and if drm_atomic_helper_shutdown() is called afterwards it's not
> >> allowed to touch hardware.
> >>
> >> I know it's the wrong order, but the only way to do it in the right
> >> order is to have a separate function that sets unplugged:
> >>
> >>drm_dev_unregister();
> >>drm_atomic_helper_shutdown();
> >>drm_dev_set_unplugged();
> >
> > Annoying ... but yeah calling _shutdown() before we stopped userspace is
> > also not going to work. Because userspace could quickly re-enable
> > something, and then the refcounts would be all wrong again and leaking
> > objects.
> >
>
> What happens with a USB device that is unplugged with open userspace,
> will that leak objects?
> >>>
> >>> Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
> >>> as normal, the only thing that should be skipped is actually touching the
> >>> hw (as long as the driver doesn't protect too much with
> >>> drm_dev_enter/exit). So all the software updates (including refcounting
> >>> updates) will still be done. Ofc current udl is not yet atomic, so in
> >>> reality something else happens.
> >>>
> >>> And we ofc still have the same issue that if you just unload the driver,
> >>> then the hw will stay on (which might really confuse the driver on next
> >>> load, when it assumes that it only gets loaded
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Daniel Vetter writes: > > Zooming out more looking at the big picture I'd say all your work in the > past few years has enormously simplified drm for simple drivers already. > If we can't resolve this one here right now that just means you "only" > made drm 98% simpler instead of maybe 99%. It's still an epic win :-) I'd like to second this. So many of Noralf's cleanups I think "oof, that's a lot of work for a little cleanup here". But we've benefited immensely from it accumulating over the years. Thanks again! signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 06.02.2019 16.26, skrev Daniel Vetter:
> On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 05.02.2019 17.31, skrev Daniel Vetter:
>>> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
Den 05.02.2019 10.11, skrev Daniel Vetter:
> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 04.02.2019 16.41, skrev Daniel Vetter:
>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
The only thing now that makes drm_dev_unplug() special is that it sets
drm_device->unplugged. Move this code to drm_dev_unregister() so that
we
can remove drm_dev_unplug().
Signed-off-by: Noralf Trønnes
---
>>
>> [...]
>>
drivers/gpu/drm/drm_drv.c | 27 +++
include/drm/drm_drv.h | 10 --
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 05bbc2b622fc..e0941200edc6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
*/
void drm_dev_unplug(struct drm_device *dev)
{
- /*
- * After synchronizing any critical read section is guaranteed
to see
- * the new value of ->unplugged, and any critical section which
might
- * still have seen the old value of ->unplugged is guaranteed
to have
- * finished.
- */
- dev->unplugged = true;
- synchronize_srcu(_unplug_srcu);
-
drm_dev_unregister(dev);
drm_dev_put(dev);
}
@@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
* drm_dev_register() but does not deallocate the device. The caller
must call
* drm_dev_put() to drop their final reference.
*
- * A special form of unregistering for hotpluggable devices is
drm_dev_unplug(),
- * which can be called while there are still open users of @dev.
+ * This function can be called while there are still open users of
@dev as long
+ * as the driver protects its device resources using drm_dev_enter()
and
+ * drm_dev_exit().
*
* This should be called first in the device teardown code to make
sure
- * userspace can't access the device instance any more.
+ * userspace can't access the device instance any more. Drivers that
support
+ * device unplug will probably want to call
drm_atomic_helper_shutdown() first
>>>
>>> Read once more with a bit more coffee, spotted this:
>>>
>>> s/first/afterwards/ - shutting down the hw before we've taken it away
>>> from
>>> userspace is kinda the wrong way round. It should be the inverse of
>>> driver
>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
>>> the world (simplified ofc).
>>>
>>
>> The problem is that drm_dev_unregister() sets the device as unplugged
>> and if drm_atomic_helper_shutdown() is called afterwards it's not
>> allowed to touch hardware.
>>
>> I know it's the wrong order, but the only way to do it in the right
>> order is to have a separate function that sets unplugged:
>>
>> drm_dev_unregister();
>> drm_atomic_helper_shutdown();
>> drm_dev_set_unplugged();
>
> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> also not going to work. Because userspace could quickly re-enable
> something, and then the refcounts would be all wrong again and leaking
> objects.
>
What happens with a USB device that is unplugged with open userspace,
will that leak objects?
>>>
>>> Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
>>> as normal, the only thing that should be skipped is actually touching the
>>> hw (as long as the driver doesn't protect too much with
>>> drm_dev_enter/exit). So all the software updates (including refcounting
>>> updates) will still be done. Ofc current udl is not yet atomic, so in
>>> reality something else happens.
>>>
>>> And we ofc still have the same issue that if you just unload the driver,
>>> then the hw will stay on (which might really confuse the driver on next
>>> load, when it assumes that it only gets loaded from cold boot where
>>> everything is off - which usually is the case on an arm soc at least).
>>>
> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> step forward already, and will open up a lot of TODO
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Tue, Feb 05, 2019 at 06:57:50PM +0100, Noralf Trønnes wrote:
>
>
> Den 05.02.2019 17.31, skrev Daniel Vetter:
> > On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 05.02.2019 10.11, skrev Daniel Vetter:
> >>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>
>
> Den 04.02.2019 16.41, skrev Daniel Vetter:
> > On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >> The only thing now that makes drm_dev_unplug() special is that it sets
> >> drm_device->unplugged. Move this code to drm_dev_unregister() so that
> >> we
> >> can remove drm_dev_unplug().
> >>
> >> Signed-off-by: Noralf Trønnes
> >> ---
>
> [...]
>
> >> drivers/gpu/drm/drm_drv.c | 27 +++
> >> include/drm/drm_drv.h | 10 --
> >> 2 files changed, 19 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 05bbc2b622fc..e0941200edc6 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >> */
> >> void drm_dev_unplug(struct drm_device *dev)
> >> {
> >> - /*
> >> - * After synchronizing any critical read section is guaranteed
> >> to see
> >> - * the new value of ->unplugged, and any critical section which
> >> might
> >> - * still have seen the old value of ->unplugged is guaranteed
> >> to have
> >> - * finished.
> >> - */
> >> - dev->unplugged = true;
> >> - synchronize_srcu(_unplug_srcu);
> >> -
> >>drm_dev_unregister(dev);
> >>drm_dev_put(dev);
> >> }
> >> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >> * drm_dev_register() but does not deallocate the device. The caller
> >> must call
> >> * drm_dev_put() to drop their final reference.
> >> *
> >> - * A special form of unregistering for hotpluggable devices is
> >> drm_dev_unplug(),
> >> - * which can be called while there are still open users of @dev.
> >> + * This function can be called while there are still open users of
> >> @dev as long
> >> + * as the driver protects its device resources using drm_dev_enter()
> >> and
> >> + * drm_dev_exit().
> >> *
> >> * This should be called first in the device teardown code to make
> >> sure
> >> - * userspace can't access the device instance any more.
> >> + * userspace can't access the device instance any more. Drivers that
> >> support
> >> + * device unplug will probably want to call
> >> drm_atomic_helper_shutdown() first
> >
> > Read once more with a bit more coffee, spotted this:
> >
> > s/first/afterwards/ - shutting down the hw before we've taken it away
> > from
> > userspace is kinda the wrong way round. It should be the inverse of
> > driver
> > load, which is 1) allocate structures 2) prep hw 3) register driver with
> > the world (simplified ofc).
> >
>
> The problem is that drm_dev_unregister() sets the device as unplugged
> and if drm_atomic_helper_shutdown() is called afterwards it's not
> allowed to touch hardware.
>
> I know it's the wrong order, but the only way to do it in the right
> order is to have a separate function that sets unplugged:
>
> drm_dev_unregister();
> drm_atomic_helper_shutdown();
> drm_dev_set_unplugged();
> >>>
> >>> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> >>> also not going to work. Because userspace could quickly re-enable
> >>> something, and then the refcounts would be all wrong again and leaking
> >>> objects.
> >>>
> >>
> >> What happens with a USB device that is unplugged with open userspace,
> >> will that leak objects?
> >
> > Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
> > as normal, the only thing that should be skipped is actually touching the
> > hw (as long as the driver doesn't protect too much with
> > drm_dev_enter/exit). So all the software updates (including refcounting
> > updates) will still be done. Ofc current udl is not yet atomic, so in
> > reality something else happens.
> >
> > And we ofc still have the same issue that if you just unload the driver,
> > then the hw will stay on (which might really confuse the driver on next
> > load, when it assumes that it only gets loaded from cold boot where
> > everything is off - which usually is the case on an arm soc at least).
> >
> >>> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> >>> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> >>> step forward already, and will open up a lot of TODO items across a lot of
> >>> drivers. E.g. we
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 05.02.2019 17.31, skrev Daniel Vetter:
> On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 05.02.2019 10.11, skrev Daniel Vetter:
>>> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
Den 04.02.2019 16.41, skrev Daniel Vetter:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>> The only thing now that makes drm_dev_unplug() special is that it sets
>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>> can remove drm_dev_unplug().
>>
>> Signed-off-by: Noralf Trønnes
>> ---
[...]
>> drivers/gpu/drm/drm_drv.c | 27 +++
>> include/drm/drm_drv.h | 10 --
>> 2 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 05bbc2b622fc..e0941200edc6 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>> */
>> void drm_dev_unplug(struct drm_device *dev)
>> {
>> -/*
>> - * After synchronizing any critical read section is guaranteed
>> to see
>> - * the new value of ->unplugged, and any critical section which
>> might
>> - * still have seen the old value of ->unplugged is guaranteed
>> to have
>> - * finished.
>> - */
>> -dev->unplugged = true;
>> -synchronize_srcu(_unplug_srcu);
>> -
>> drm_dev_unregister(dev);
>> drm_dev_put(dev);
>> }
>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>> * drm_dev_register() but does not deallocate the device. The caller
>> must call
>> * drm_dev_put() to drop their final reference.
>> *
>> - * A special form of unregistering for hotpluggable devices is
>> drm_dev_unplug(),
>> - * which can be called while there are still open users of @dev.
>> + * This function can be called while there are still open users of @dev
>> as long
>> + * as the driver protects its device resources using drm_dev_enter() and
>> + * drm_dev_exit().
>> *
>> * This should be called first in the device teardown code to make sure
>> - * userspace can't access the device instance any more.
>> + * userspace can't access the device instance any more. Drivers that
>> support
>> + * device unplug will probably want to call
>> drm_atomic_helper_shutdown() first
>
> Read once more with a bit more coffee, spotted this:
>
> s/first/afterwards/ - shutting down the hw before we've taken it away from
> userspace is kinda the wrong way round. It should be the inverse of driver
> load, which is 1) allocate structures 2) prep hw 3) register driver with
> the world (simplified ofc).
>
The problem is that drm_dev_unregister() sets the device as unplugged
and if drm_atomic_helper_shutdown() is called afterwards it's not
allowed to touch hardware.
I know it's the wrong order, but the only way to do it in the right
order is to have a separate function that sets unplugged:
drm_dev_unregister();
drm_atomic_helper_shutdown();
drm_dev_set_unplugged();
>>>
>>> Annoying ... but yeah calling _shutdown() before we stopped userspace is
>>> also not going to work. Because userspace could quickly re-enable
>>> something, and then the refcounts would be all wrong again and leaking
>>> objects.
>>>
>>
>> What happens with a USB device that is unplugged with open userspace,
>> will that leak objects?
>
> Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
> as normal, the only thing that should be skipped is actually touching the
> hw (as long as the driver doesn't protect too much with
> drm_dev_enter/exit). So all the software updates (including refcounting
> updates) will still be done. Ofc current udl is not yet atomic, so in
> reality something else happens.
>
> And we ofc still have the same issue that if you just unload the driver,
> then the hw will stay on (which might really confuse the driver on next
> load, when it assumes that it only gets loaded from cold boot where
> everything is off - which usually is the case on an arm soc at least).
>
>>> I get a bit the feeling we're over-optimizing here with trying to devm-ize
>>> drm_dev_register. Just getting drm_device correctly devm-ized is a big
>>> step forward already, and will open up a lot of TODO items across a lot of
>>> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
>>> structs, which gets released together with drm_device. I think that's a
>>> much clearer path forward, I think we all agree that getting the kfree out
>>> of the driver codes is a good thing, and it would allow us to do this
>>> correctly.
>>>
>>>
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Tue, Feb 05, 2019 at 11:20:55AM +0100, Noralf Trønnes wrote:
>
>
> Den 05.02.2019 10.11, skrev Daniel Vetter:
> > On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> >>
> >>
> >> Den 04.02.2019 16.41, skrev Daniel Vetter:
> >>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes
> ---
> >>
> >> [...]
> >>
> drivers/gpu/drm/drm_drv.c | 27 +++
> include/drm/drm_drv.h | 10 --
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> */
> void drm_dev_unplug(struct drm_device *dev)
> {
> -/*
> - * After synchronizing any critical read section is guaranteed
> to see
> - * the new value of ->unplugged, and any critical section which
> might
> - * still have seen the old value of ->unplugged is guaranteed
> to have
> - * finished.
> - */
> -dev->unplugged = true;
> -synchronize_srcu(_unplug_srcu);
> -
> drm_dev_unregister(dev);
> drm_dev_put(dev);
> }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> * drm_dev_register() but does not deallocate the device. The caller
> must call
> * drm_dev_put() to drop their final reference.
> *
> - * A special form of unregistering for hotpluggable devices is
> drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev
> as long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
> *
> * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that
> support
> + * device unplug will probably want to call
> drm_atomic_helper_shutdown() first
> >>>
> >>> Read once more with a bit more coffee, spotted this:
> >>>
> >>> s/first/afterwards/ - shutting down the hw before we've taken it away from
> >>> userspace is kinda the wrong way round. It should be the inverse of driver
> >>> load, which is 1) allocate structures 2) prep hw 3) register driver with
> >>> the world (simplified ofc).
> >>>
> >>
> >> The problem is that drm_dev_unregister() sets the device as unplugged
> >> and if drm_atomic_helper_shutdown() is called afterwards it's not
> >> allowed to touch hardware.
> >>
> >> I know it's the wrong order, but the only way to do it in the right
> >> order is to have a separate function that sets unplugged:
> >>
> >>drm_dev_unregister();
> >>drm_atomic_helper_shutdown();
> >>drm_dev_set_unplugged();
> >
> > Annoying ... but yeah calling _shutdown() before we stopped userspace is
> > also not going to work. Because userspace could quickly re-enable
> > something, and then the refcounts would be all wrong again and leaking
> > objects.
> >
>
> What happens with a USB device that is unplugged with open userspace,
> will that leak objects?
Maybe we've jumped to conclusions. drm_atomic_helper_shutdown() will run
as normal, the only thing that should be skipped is actually touching the
hw (as long as the driver doesn't protect too much with
drm_dev_enter/exit). So all the software updates (including refcounting
updates) will still be done. Ofc current udl is not yet atomic, so in
reality something else happens.
And we ofc still have the same issue that if you just unload the driver,
then the hw will stay on (which might really confuse the driver on next
load, when it assumes that it only gets loaded from cold boot where
everything is off - which usually is the case on an arm soc at least).
> > I get a bit the feeling we're over-optimizing here with trying to devm-ize
> > drm_dev_register. Just getting drm_device correctly devm-ized is a big
> > step forward already, and will open up a lot of TODO items across a lot of
> > drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> > structs, which gets released together with drm_device. I think that's a
> > much clearer path forward, I think we all agree that getting the kfree out
> > of the driver codes is a good thing, and it would allow us to do this
> > correctly.
> >
> > Then once we have that and rolled out to a few drivers we can reconsider
>
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 05.02.2019 10.11, skrev Daniel Vetter:
> On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 04.02.2019 16.41, skrev Daniel Vetter:
>>> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
The only thing now that makes drm_dev_unplug() special is that it sets
drm_device->unplugged. Move this code to drm_dev_unregister() so that we
can remove drm_dev_unplug().
Signed-off-by: Noralf Trønnes
---
>>
>> [...]
>>
drivers/gpu/drm/drm_drv.c | 27 +++
include/drm/drm_drv.h | 10 --
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 05bbc2b622fc..e0941200edc6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
*/
void drm_dev_unplug(struct drm_device *dev)
{
- /*
- * After synchronizing any critical read section is guaranteed to see
- * the new value of ->unplugged, and any critical section which might
- * still have seen the old value of ->unplugged is guaranteed to have
- * finished.
- */
- dev->unplugged = true;
- synchronize_srcu(_unplug_srcu);
-
drm_dev_unregister(dev);
drm_dev_put(dev);
}
@@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
* drm_dev_register() but does not deallocate the device. The caller must
call
* drm_dev_put() to drop their final reference.
*
- * A special form of unregistering for hotpluggable devices is
drm_dev_unplug(),
- * which can be called while there are still open users of @dev.
+ * This function can be called while there are still open users of @dev
as long
+ * as the driver protects its device resources using drm_dev_enter() and
+ * drm_dev_exit().
*
* This should be called first in the device teardown code to make sure
- * userspace can't access the device instance any more.
+ * userspace can't access the device instance any more. Drivers that
support
+ * device unplug will probably want to call drm_atomic_helper_shutdown()
first
>>>
>>> Read once more with a bit more coffee, spotted this:
>>>
>>> s/first/afterwards/ - shutting down the hw before we've taken it away from
>>> userspace is kinda the wrong way round. It should be the inverse of driver
>>> load, which is 1) allocate structures 2) prep hw 3) register driver with
>>> the world (simplified ofc).
>>>
>>
>> The problem is that drm_dev_unregister() sets the device as unplugged
>> and if drm_atomic_helper_shutdown() is called afterwards it's not
>> allowed to touch hardware.
>>
>> I know it's the wrong order, but the only way to do it in the right
>> order is to have a separate function that sets unplugged:
>>
>> drm_dev_unregister();
>> drm_atomic_helper_shutdown();
>> drm_dev_set_unplugged();
>
> Annoying ... but yeah calling _shutdown() before we stopped userspace is
> also not going to work. Because userspace could quickly re-enable
> something, and then the refcounts would be all wrong again and leaking
> objects.
>
What happens with a USB device that is unplugged with open userspace,
will that leak objects?
> I get a bit the feeling we're over-optimizing here with trying to devm-ize
> drm_dev_register. Just getting drm_device correctly devm-ized is a big
> step forward already, and will open up a lot of TODO items across a lot of
> drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
> structs, which gets released together with drm_device. I think that's a
> much clearer path forward, I think we all agree that getting the kfree out
> of the driver codes is a good thing, and it would allow us to do this
> correctly.
>
> Then once we have that and rolled out to a few drivers we can reconsider
> the entire unregister/shutdown gordian knot here. Atm I just have no idea
> how to do this properly :-/
>
> Thoughts, other ideas?
>
Yeah, I've come to the conclusion that devm_drm_dev_register() doesn't
make much sense if we need a driver remove callback anyways.
I think devm_drm_dev_init() makes sense because it yields a cleaner
probe() function. An additional benefit is that it requires a
drm_driver->release function which is a step in the right direction to
get the drm_device lifetime right.
Do we agree that a drm_dev_set_unplugged() function is necessary to get
the remove/disconnect order right?
What about drm_dev_unplug() maybe I should just leave it be?
- amd uses drm_driver->unload, so that one takes some work to get right
to support unplug. It doesn't check the unplugged state, so really
doesn't need drm_dev_unplug() I guess. Do they have cards that can be
hotplugged?
- udl uses drm_driver->unload, doesn't use drm_atomic_helper_shutdown().
It has only one
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
>
>
> Den 04.02.2019 16.41, skrev Daniel Vetter:
> > On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >> The only thing now that makes drm_dev_unplug() special is that it sets
> >> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> >> can remove drm_dev_unplug().
> >>
> >> Signed-off-by: Noralf Trønnes
> >> ---
>
> [...]
>
> >> drivers/gpu/drm/drm_drv.c | 27 +++
> >> include/drm/drm_drv.h | 10 --
> >> 2 files changed, 19 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 05bbc2b622fc..e0941200edc6 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >> */
> >> void drm_dev_unplug(struct drm_device *dev)
> >> {
> >> - /*
> >> - * After synchronizing any critical read section is guaranteed to see
> >> - * the new value of ->unplugged, and any critical section which might
> >> - * still have seen the old value of ->unplugged is guaranteed to have
> >> - * finished.
> >> - */
> >> - dev->unplugged = true;
> >> - synchronize_srcu(_unplug_srcu);
> >> -
> >>drm_dev_unregister(dev);
> >>drm_dev_put(dev);
> >> }
> >> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >> * drm_dev_register() but does not deallocate the device. The caller must
> >> call
> >> * drm_dev_put() to drop their final reference.
> >> *
> >> - * A special form of unregistering for hotpluggable devices is
> >> drm_dev_unplug(),
> >> - * which can be called while there are still open users of @dev.
> >> + * This function can be called while there are still open users of @dev
> >> as long
> >> + * as the driver protects its device resources using drm_dev_enter() and
> >> + * drm_dev_exit().
> >> *
> >> * This should be called first in the device teardown code to make sure
> >> - * userspace can't access the device instance any more.
> >> + * userspace can't access the device instance any more. Drivers that
> >> support
> >> + * device unplug will probably want to call drm_atomic_helper_shutdown()
> >> first
> >
> > Read once more with a bit more coffee, spotted this:
> >
> > s/first/afterwards/ - shutting down the hw before we've taken it away from
> > userspace is kinda the wrong way round. It should be the inverse of driver
> > load, which is 1) allocate structures 2) prep hw 3) register driver with
> > the world (simplified ofc).
> >
>
> The problem is that drm_dev_unregister() sets the device as unplugged
> and if drm_atomic_helper_shutdown() is called afterwards it's not
> allowed to touch hardware.
>
> I know it's the wrong order, but the only way to do it in the right
> order is to have a separate function that sets unplugged:
>
> drm_dev_unregister();
> drm_atomic_helper_shutdown();
> drm_dev_set_unplugged();
Annoying ... but yeah calling _shutdown() before we stopped userspace is
also not going to work. Because userspace could quickly re-enable
something, and then the refcounts would be all wrong again and leaking
objects.
I get a bit the feeling we're over-optimizing here with trying to devm-ize
drm_dev_register. Just getting drm_device correctly devm-ized is a big
step forward already, and will open up a lot of TODO items across a lot of
drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
structs, which gets released together with drm_device. I think that's a
much clearer path forward, I think we all agree that getting the kfree out
of the driver codes is a good thing, and it would allow us to do this
correctly.
Then once we have that and rolled out to a few drivers we can reconsider
the entire unregister/shutdown gordian knot here. Atm I just have no idea
how to do this properly :-/
Thoughts, other ideas?
Cheers, Daniel
> Noralf.
>
> >> + * in order to disable the hardware on regular driver module unload.
> >> */
> >> void drm_dev_unregister(struct drm_device *dev)
> >> {
> >> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >>if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >>drm_lastclose(dev);
> >>
> >> + /*
> >> + * After synchronizing any critical read section is guaranteed to see
> >> + * the new value of ->unplugged, and any critical section which might
> >> + * still have seen the old value of ->unplugged is guaranteed to have
> >> + * finished.
> >> + */
> >> + dev->unplugged = true;
> >> + synchronize_srcu(_unplug_srcu);
> >> +
> >>dev->registered = false;
> >>
> >>drm_client_dev_unregister(dev);
> >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >> index ca46a45a9cce..c50696c82a42 100644
> >> --- a/include/drm/drm_drv.h
> >> +++ b/include/drm/drm_drv.h
> >> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >> *
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On 2/3/19 5:41 PM, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes
Reviewed-by: Oleksandr Andrushchenko
> ---
>
> Maybe s/unplugged/unregistered/ ?
>
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.
>
> Noralf.
>
>
> drivers/gpu/drm/drm_drv.c | 27 +++
> include/drm/drm_drv.h | 10 --
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>*/
> void drm_dev_unplug(struct drm_device *dev)
> {
> - /*
> - * After synchronizing any critical read section is guaranteed to see
> - * the new value of ->unplugged, and any critical section which might
> - * still have seen the old value of ->unplugged is guaranteed to have
> - * finished.
> - */
> - dev->unplugged = true;
> - synchronize_srcu(_unplug_srcu);
> -
> drm_dev_unregister(dev);
> drm_dev_put(dev);
> }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>* drm_dev_register() but does not deallocate the device. The caller must
> call
>* drm_dev_put() to drop their final reference.
>*
> - * A special form of unregistering for hotpluggable devices is
> drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as
> long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
>*
>* This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown()
> first
> + * in order to disable the hardware on regular driver module unload.
>*/
> void drm_dev_unregister(struct drm_device *dev)
> {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> if (drm_core_check_feature(dev, DRIVER_LEGACY))
> drm_lastclose(dev);
>
> + /*
> + * After synchronizing any critical read section is guaranteed to see
> + * the new value of ->unplugged, and any critical section which might
> + * still have seen the old value of ->unplugged is guaranteed to have
> + * finished.
> + */
> + dev->unplugged = true;
> + synchronize_srcu(_unplug_srcu);
> +
> dev->registered = false;
>
> drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>* drm_dev_is_unplugged - is a DRM device unplugged
>* @dev: DRM device
>*
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This
> can
> + * be used to detect that the underlying parent device is gone.
>*
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the underlying drm_dev_enter() and
> + * WARNING: This function fundamentally races against drm_dev_unregister().
> It
> + * is recommended that drivers instead use the underlying drm_dev_enter() and
>* drm_dev_exit() function pairs.
>*/
> static inline bool drm_dev_is_unplugged(struct drm_device *dev)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
Den 04.02.2019 16.41, skrev Daniel Vetter:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
>> The only thing now that makes drm_dev_unplug() special is that it sets
>> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
>> can remove drm_dev_unplug().
>>
>> Signed-off-by: Noralf Trønnes
>> ---
[...]
>> drivers/gpu/drm/drm_drv.c | 27 +++
>> include/drm/drm_drv.h | 10 --
>> 2 files changed, 19 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 05bbc2b622fc..e0941200edc6 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
>> */
>> void drm_dev_unplug(struct drm_device *dev)
>> {
>> -/*
>> - * After synchronizing any critical read section is guaranteed to see
>> - * the new value of ->unplugged, and any critical section which might
>> - * still have seen the old value of ->unplugged is guaranteed to have
>> - * finished.
>> - */
>> -dev->unplugged = true;
>> -synchronize_srcu(_unplug_srcu);
>> -
>> drm_dev_unregister(dev);
>> drm_dev_put(dev);
>> }
>> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
>> * drm_dev_register() but does not deallocate the device. The caller must
>> call
>> * drm_dev_put() to drop their final reference.
>> *
>> - * A special form of unregistering for hotpluggable devices is
>> drm_dev_unplug(),
>> - * which can be called while there are still open users of @dev.
>> + * This function can be called while there are still open users of @dev as
>> long
>> + * as the driver protects its device resources using drm_dev_enter() and
>> + * drm_dev_exit().
>> *
>> * This should be called first in the device teardown code to make sure
>> - * userspace can't access the device instance any more.
>> + * userspace can't access the device instance any more. Drivers that support
>> + * device unplug will probably want to call drm_atomic_helper_shutdown()
>> first
>
> Read once more with a bit more coffee, spotted this:
>
> s/first/afterwards/ - shutting down the hw before we've taken it away from
> userspace is kinda the wrong way round. It should be the inverse of driver
> load, which is 1) allocate structures 2) prep hw 3) register driver with
> the world (simplified ofc).
>
The problem is that drm_dev_unregister() sets the device as unplugged
and if drm_atomic_helper_shutdown() is called afterwards it's not
allowed to touch hardware.
I know it's the wrong order, but the only way to do it in the right
order is to have a separate function that sets unplugged:
drm_dev_unregister();
drm_atomic_helper_shutdown();
drm_dev_set_unplugged();
Noralf.
>> + * in order to disable the hardware on regular driver module unload.
>> */
>> void drm_dev_unregister(struct drm_device *dev)
>> {
>> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
>> if (drm_core_check_feature(dev, DRIVER_LEGACY))
>> drm_lastclose(dev);
>>
>> +/*
>> + * After synchronizing any critical read section is guaranteed to see
>> + * the new value of ->unplugged, and any critical section which might
>> + * still have seen the old value of ->unplugged is guaranteed to have
>> + * finished.
>> + */
>> +dev->unplugged = true;
>> +synchronize_srcu(_unplug_srcu);
>> +
>> dev->registered = false;
>>
>> drm_client_dev_unregister(dev);
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index ca46a45a9cce..c50696c82a42 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
>> * drm_dev_is_unplugged - is a DRM device unplugged
>> * @dev: DRM device
>> *
>> - * This function can be called to check whether a hotpluggable is unplugged.
>> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
>> - * unplugged, these two functions guarantee that any store before calling
>> - * drm_dev_unplug() is visible to callers of this function after it
>> completes
>> + * This function can be called to check whether @dev is unregistered. This
>> can
>> + * be used to detect that the underlying parent device is gone.
>
> I think it'd be good to keep the first part, and just update the reference
> to drm_dev_unregister. So:
>
> * This function can be called to check whether a hotpluggable is unplugged.
> * Unplugging itself is singalled through drm_dev_unregister(). If a device is
> * unplugged, these two functions guarantee that any store before calling
> * drm_dev_unregister() is visible to callers of this function after it
> * completes.
>
> I think your version shrugs a few important details under the rug. With
> those nits addressed:
>
> Reviewed-by: Daniel Vetter
>
> Cheers, Daniel
>
>> *
>> - * WARNING: This function
Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes
> ---
>
> Maybe s/unplugged/unregistered/ ?
>
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.
Yeah I think we need to keep the registered state separate from unplugged.
Iirc this exact scenario is what we discussed when you revamped the
unplug infrastructure.
>
> Noralf.
>
>
> drivers/gpu/drm/drm_drv.c | 27 +++
> include/drm/drm_drv.h | 10 --
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> */
> void drm_dev_unplug(struct drm_device *dev)
> {
> - /*
> - * After synchronizing any critical read section is guaranteed to see
> - * the new value of ->unplugged, and any critical section which might
> - * still have seen the old value of ->unplugged is guaranteed to have
> - * finished.
> - */
> - dev->unplugged = true;
> - synchronize_srcu(_unplug_srcu);
> -
> drm_dev_unregister(dev);
> drm_dev_put(dev);
> }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> * drm_dev_register() but does not deallocate the device. The caller must
> call
> * drm_dev_put() to drop their final reference.
> *
> - * A special form of unregistering for hotpluggable devices is
> drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as
> long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
> *
> * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown()
> first
Read once more with a bit more coffee, spotted this:
s/first/afterwards/ - shutting down the hw before we've taken it away from
userspace is kinda the wrong way round. It should be the inverse of driver
load, which is 1) allocate structures 2) prep hw 3) register driver with
the world (simplified ofc).
> + * in order to disable the hardware on regular driver module unload.
> */
> void drm_dev_unregister(struct drm_device *dev)
> {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> if (drm_core_check_feature(dev, DRIVER_LEGACY))
> drm_lastclose(dev);
>
> + /*
> + * After synchronizing any critical read section is guaranteed to see
> + * the new value of ->unplugged, and any critical section which might
> + * still have seen the old value of ->unplugged is guaranteed to have
> + * finished.
> + */
> + dev->unplugged = true;
> + synchronize_srcu(_unplug_srcu);
> +
> dev->registered = false;
>
> drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> * drm_dev_is_unplugged - is a DRM device unplugged
> * @dev: DRM device
> *
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This
> can
> + * be used to detect that the underlying parent device is gone.
I think it'd be good to keep the first part, and just update the reference
to drm_dev_unregister. So:
* This function can be called to check whether a hotpluggable is unplugged.
* Unplugging itself is singalled through drm_dev_unregister(). If a device is
* unplugged, these two functions guarantee that any store before calling
* drm_dev_unregister() is visible to callers of this function after it
* completes.
I think your version shrugs a few important details under the rug. With
those nits addressed:
Reviewed-by: Daniel Vetter
Cheers, Daniel
> *
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the
[Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
The only thing now that makes drm_dev_unplug() special is that it sets
drm_device->unplugged. Move this code to drm_dev_unregister() so that we
can remove drm_dev_unplug().
Signed-off-by: Noralf Trønnes
---
Maybe s/unplugged/unregistered/ ?
I looked at drm_device->registered, but using that would mean that
drm_dev_is_unplugged() would return before drm_device is registered.
And given that its current purpose is to prevent race against connector
registration, I stayed away from it.
Noralf.
drivers/gpu/drm/drm_drv.c | 27 +++
include/drm/drm_drv.h | 10 --
2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 05bbc2b622fc..e0941200edc6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
*/
void drm_dev_unplug(struct drm_device *dev)
{
- /*
-* After synchronizing any critical read section is guaranteed to see
-* the new value of ->unplugged, and any critical section which might
-* still have seen the old value of ->unplugged is guaranteed to have
-* finished.
-*/
- dev->unplugged = true;
- synchronize_srcu(_unplug_srcu);
-
drm_dev_unregister(dev);
drm_dev_put(dev);
}
@@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
* drm_dev_register() but does not deallocate the device. The caller must call
* drm_dev_put() to drop their final reference.
*
- * A special form of unregistering for hotpluggable devices is
drm_dev_unplug(),
- * which can be called while there are still open users of @dev.
+ * This function can be called while there are still open users of @dev as long
+ * as the driver protects its device resources using drm_dev_enter() and
+ * drm_dev_exit().
*
* This should be called first in the device teardown code to make sure
- * userspace can't access the device instance any more.
+ * userspace can't access the device instance any more. Drivers that support
+ * device unplug will probably want to call drm_atomic_helper_shutdown() first
+ * in order to disable the hardware on regular driver module unload.
*/
void drm_dev_unregister(struct drm_device *dev)
{
@@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_LEGACY))
drm_lastclose(dev);
+ /*
+* After synchronizing any critical read section is guaranteed to see
+* the new value of ->unplugged, and any critical section which might
+* still have seen the old value of ->unplugged is guaranteed to have
+* finished.
+*/
+ dev->unplugged = true;
+ synchronize_srcu(_unplug_srcu);
+
dev->registered = false;
drm_client_dev_unregister(dev);
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index ca46a45a9cce..c50696c82a42 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
* drm_dev_is_unplugged - is a DRM device unplugged
* @dev: DRM device
*
- * This function can be called to check whether a hotpluggable is unplugged.
- * Unplugging itself is singalled through drm_dev_unplug(). If a device is
- * unplugged, these two functions guarantee that any store before calling
- * drm_dev_unplug() is visible to callers of this function after it completes
+ * This function can be called to check whether @dev is unregistered. This can
+ * be used to detect that the underlying parent device is gone.
*
- * WARNING: This function fundamentally races against drm_dev_unplug(). It is
- * recommended that drivers instead use the underlying drm_dev_enter() and
+ * WARNING: This function fundamentally races against drm_dev_unregister(). It
+ * is recommended that drivers instead use the underlying drm_dev_enter() and
* drm_dev_exit() function pairs.
*/
static inline bool drm_dev_is_unplugged(struct drm_device *dev)
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
