| Message ID | 20200507150822.114464-5-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fareless gem_free_object | expand |
Hi Emil. On Thu, May 07, 2020 at 04:07:50PM +0100, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > There's little point in providing partial and ancient information about > the struct_mutex. Some drivers are using it, new ones should not. > > As-it this only provides for confusion. > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > Documentation/gpu/drm-mm.rst | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > index 1839762044be..5ba2ead8f317 100644 > --- a/Documentation/gpu/drm-mm.rst > +++ b/Documentation/gpu/drm-mm.rst > @@ -178,11 +178,8 @@ GEM Objects Lifetime > -------------------- > > All GEM objects are reference-counted by the GEM core. References can be > -acquired and release by calling drm_gem_object_get() and drm_gem_object_put() > -respectively. The caller must hold the :c:type:`struct drm_device <drm_device>` > -struct_mutex lock when calling drm_gem_object_get(). As a convenience, GEM > -provides drm_gem_object_put_unlocked() functions that can be called without > -holding the lock. > +acquired and release by calling drm_gem_object_get() and drm_gem_object_put_unlocked() > +respectively. Nice to get rid of struct_mutex lock stuff. But no need to s/drm_gem_object_put/drm_gem_object_put_unlocked()/ as this will be renamed a bit later. Sam > > When the last reference to a GEM object is released the GEM core calls > the :c:type:`struct drm_driver <drm_driver>` gem_free_object_unlocked > -- > 2.25.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 07, 2020 at 04:07:50PM +0100, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > There's little point in providing partial and ancient information about > the struct_mutex. Some drivers are using it, new ones should not. > > As-it this only provides for confusion. > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> I think a doc patch to add a big warning for drm_device.struct_mutex would also be good. The current text is kinda unhelpful. -Daniel > --- > Documentation/gpu/drm-mm.rst | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > index 1839762044be..5ba2ead8f317 100644 > --- a/Documentation/gpu/drm-mm.rst > +++ b/Documentation/gpu/drm-mm.rst > @@ -178,11 +178,8 @@ GEM Objects Lifetime > -------------------- > > All GEM objects are reference-counted by the GEM core. References can be > -acquired and release by calling drm_gem_object_get() and drm_gem_object_put() > -respectively. The caller must hold the :c:type:`struct drm_device <drm_device>` > -struct_mutex lock when calling drm_gem_object_get(). As a convenience, GEM > -provides drm_gem_object_put_unlocked() functions that can be called without > -holding the lock. > +acquired and release by calling drm_gem_object_get() and drm_gem_object_put_unlocked() > +respectively. > > When the last reference to a GEM object is released the GEM core calls > the :c:type:`struct drm_driver <drm_driver>` gem_free_object_unlocked > -- > 2.25.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sam, On Thu, 7 May 2020 at 19:01, Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Emil. > > On Thu, May 07, 2020 at 04:07:50PM +0100, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > There's little point in providing partial and ancient information about > > the struct_mutex. Some drivers are using it, new ones should not. > > > > As-it this only provides for confusion. > > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > --- > > Documentation/gpu/drm-mm.rst | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > > index 1839762044be..5ba2ead8f317 100644 > > --- a/Documentation/gpu/drm-mm.rst > > +++ b/Documentation/gpu/drm-mm.rst > > @@ -178,11 +178,8 @@ GEM Objects Lifetime > > -------------------- > > > > All GEM objects are reference-counted by the GEM core. References can be > > -acquired and release by calling drm_gem_object_get() and drm_gem_object_put() > > -respectively. The caller must hold the :c:type:`struct drm_device <drm_device>` > > -struct_mutex lock when calling drm_gem_object_get(). As a convenience, GEM > > -provides drm_gem_object_put_unlocked() functions that can be called without > > -holding the lock. > > +acquired and release by calling drm_gem_object_get() and drm_gem_object_put_unlocked() > > +respectively. > > Nice to get rid of struct_mutex lock stuff. > But no need to s/drm_gem_object_put/drm_gem_object_put_unlocked()/ as this will > be renamed a bit later. > This patch fixes the documentation, for people looking it today. While I would love to see the s/_unlocked//g part of the series land, it is rather invasive albeit mechanical. So driver maintainers are in their right to request that we push it at a later point. Thus it makes perfect sense to address the two things in separate patches. -Emil
On Fri, 8 May 2020 at 07:27, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, May 07, 2020 at 04:07:50PM +0100, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > There's little point in providing partial and ancient information about > > the struct_mutex. Some drivers are using it, new ones should not. > > > > As-it this only provides for confusion. > > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > I think a doc patch to add a big warning for drm_device.struct_mutex would > also be good. The current text is kinda unhelpful. Would something like this be enough? diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index a55874db9dd4..0988351d743c 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -146,6 +146,9 @@ struct drm_device { * @struct_mutex: * * Lock for others (not &drm_minor.master and &drm_file.is_master) + * + * WARNING: + * Only drivers annotated with DRIVER_LEGACY should be using this. */ struct mutex struct_mutex; -Emil
On Fri, May 08, 2020 at 11:07:00AM +0100, Emil Velikov wrote: > On Fri, 8 May 2020 at 07:27, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Thu, May 07, 2020 at 04:07:50PM +0100, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > There's little point in providing partial and ancient information about > > > the struct_mutex. Some drivers are using it, new ones should not. > > > > > > As-it this only provides for confusion. > > > > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > > I think a doc patch to add a big warning for drm_device.struct_mutex would > > also be good. The current text is kinda unhelpful. > > Would something like this be enough? > > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index a55874db9dd4..0988351d743c 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -146,6 +146,9 @@ struct drm_device { > * @struct_mutex: > * > * Lock for others (not &drm_minor.master and &drm_file.is_master) > + * > + * WARNING: > + * Only drivers annotated with DRIVER_LEGACY should be using this. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Assuming you wrap it in some suitable commit and all that. -Daniel > */ > struct mutex struct_mutex; > > -Emil
Hi Emil. On Fri, May 08, 2020 at 11:01:25AM +0100, Emil Velikov wrote: > Hi Sam, > > On Thu, 7 May 2020 at 19:01, Sam Ravnborg <sam@ravnborg.org> wrote: > > > > Hi Emil. > > > > On Thu, May 07, 2020 at 04:07:50PM +0100, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > There's little point in providing partial and ancient information about > > > the struct_mutex. Some drivers are using it, new ones should not. > > > > > > As-it this only provides for confusion. > > > > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > --- > > > Documentation/gpu/drm-mm.rst | 7 ++----- > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > > > index 1839762044be..5ba2ead8f317 100644 > > > --- a/Documentation/gpu/drm-mm.rst > > > +++ b/Documentation/gpu/drm-mm.rst > > > @@ -178,11 +178,8 @@ GEM Objects Lifetime > > > -------------------- > > > > > > All GEM objects are reference-counted by the GEM core. References can be > > > -acquired and release by calling drm_gem_object_get() and drm_gem_object_put() > > > -respectively. The caller must hold the :c:type:`struct drm_device <drm_device>` > > > -struct_mutex lock when calling drm_gem_object_get(). As a convenience, GEM > > > -provides drm_gem_object_put_unlocked() functions that can be called without > > > -holding the lock. > > > +acquired and release by calling drm_gem_object_get() and drm_gem_object_put_unlocked() > > > +respectively. > > > > Nice to get rid of struct_mutex lock stuff. > > But no need to s/drm_gem_object_put/drm_gem_object_put_unlocked()/ as this will > > be renamed a bit later. > > > This patch fixes the documentation, for people looking it today. > > While I would love to see the s/_unlocked//g part of the series land, > it is rather invasive albeit mechanical. > So driver maintainers are in their right to request that we push it at > a later point. Well, unless there is push-back within a week from one of the maintainers then we should apply the full series at drm-misc-next. Maybe wiht a gently ping in mid next week. No reason to wait for individual maintaintes to pick it up driver-by-driver. This would make such nice refactoring as this far to hard to do especially due to the dependencies to the first patches. But I see your rationale for keeping it like it is. So you can stick an Acked-by: Sam Ravnborg <sam@ravnborg.org> on the rest of the patches. Sam
diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index 1839762044be..5ba2ead8f317 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -178,11 +178,8 @@ GEM Objects Lifetime -------------------- All GEM objects are reference-counted by the GEM core. References can be -acquired and release by calling drm_gem_object_get() and drm_gem_object_put() -respectively. The caller must hold the :c:type:`struct drm_device <drm_device>` -struct_mutex lock when calling drm_gem_object_get(). As a convenience, GEM -provides drm_gem_object_put_unlocked() functions that can be called without -holding the lock. +acquired and release by calling drm_gem_object_get() and drm_gem_object_put_unlocked() +respectively. When the last reference to a GEM object is released the GEM core calls the :c:type:`struct drm_driver <drm_driver>` gem_free_object_unlocked