Re: [PATCH RFC v4 01/16] drm: Add drm_minor_for_each

[Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>]

On Tue, Sep 3, 2019 at 9:45 PM Kenny Ho <y2kenny@gmail.com> wrote:
>
> On Tue, Sep 3, 2019 at 3:57 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Aug 29, 2019 at 02:05:18AM -0400, Kenny Ho wrote:
> > > To allow other subsystems to iterate through all stored DRM minors and
> > > act upon them.
> > >
> > > Also exposes drm_minor_acquire and drm_minor_release for other subsystem
> > > to handle drm_minor.  DRM cgroup controller is the initial consumer of
> > > this new features.
> > >
> > > Change-Id: I7c4b67ce6b31f06d1037b03435386ff5b8144ca5
> > > Signed-off-by: Kenny Ho <Kenny.Ho@amd.com>
> >
> > Iterating over minors for cgroups sounds very, very wrong. Why do we care
> > whether a buffer was allocated through kms dumb vs render nodes?
> >
> > I'd expect all the cgroup stuff to only work on drm_device, if it does
> > care about devices.
> >
> > (I didn't look through the patch series to find out where exactly you're
> > using this, so maybe I'm off the rails here).
>
> I am exposing this to remove the need to keep track of a separate list
> of available drm_device in the system (to remove the registering and
> unregistering of drm_device to the cgroup subsystem and just use
> drm_minor as the single source of truth.)  I am only filtering out the
> render nodes minor because they point to the same drm_device and is
> confusing.
>
> Perhaps I missed an obvious way to list the drm devices without
> iterating through the drm_minors?  (I probably jumped to the minors
> because $major:$minor is the convention to address devices in cgroup.)

Create your own if there's nothing, because you need to anyway:
- You need special locking anyway, we can't just block on the idr lock
for everything.
- This needs to refcount drm_device, no the minors.

Iterating over stuff still feels kinda wrong still, because normally
the way we register/unregister userspace api (and cgroups isn't
anything else from a drm driver pov) is by adding more calls to
drm_dev_register/unregister. If you put a drm_cg_register/unregister
call in there we have a clean separation, and you can track all the
currently active devices however you want. Iterating over objects that
can be hotunplugged any time tends to get really complicated really
quickly.
-Daniel


>
> Kenny
>
> > -Daniel
> >
> > > ---
> > >  drivers/gpu/drm/drm_drv.c      | 19 +++++++++++++++++++
> > >  drivers/gpu/drm/drm_internal.h |  4 ----
> > >  include/drm/drm_drv.h          |  4 ++++
> > >  3 files changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 862621494a93..000cddabd970 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -254,11 +254,13 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id)
> > >
> > >       return minor;
> > >  }
> > > +EXPORT_SYMBOL(drm_minor_acquire);
> > >
> > >  void drm_minor_release(struct drm_minor *minor)
> > >  {
> > >       drm_dev_put(minor->dev);
> > >  }
> > > +EXPORT_SYMBOL(drm_minor_release);
> > >
> > >  /**
> > >   * DOC: driver instance overview
> > > @@ -1078,6 +1080,23 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
> > >  }
> > >  EXPORT_SYMBOL(drm_dev_set_unique);
> > >
> > > +/**
> > > + * drm_minor_for_each - Iterate through all stored DRM minors
> > > + * @fn: Function to be called for each pointer.
> > > + * @data: Data passed to callback function.
> > > + *
> > > + * The callback function will be called for each @drm_minor entry, passing
> > > + * the minor, the entry and @data.
> > > + *
> > > + * If @fn returns anything other than %0, the iteration stops and that
> > > + * value is returned from this function.
> > > + */
> > > +int drm_minor_for_each(int (*fn)(int id, void *p, void *data), void *data)
> > > +{
> > > +     return idr_for_each(&drm_minors_idr, fn, data);
> > > +}
> > > +EXPORT_SYMBOL(drm_minor_for_each);
> > > +
> > >  /*
> > >   * DRM Core
> > >   * The DRM core module initializes all global DRM objects and makes them
> > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > > index e19ac7ca602d..6bfad76f8e78 100644
> > > --- a/drivers/gpu/drm/drm_internal.h
> > > +++ b/drivers/gpu/drm/drm_internal.h
> > > @@ -54,10 +54,6 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv);
> > >  void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv,
> > >                                       struct dma_buf *dma_buf);
> > >
> > > -/* drm_drv.c */
> > > -struct drm_minor *drm_minor_acquire(unsigned int minor_id);
> > > -void drm_minor_release(struct drm_minor *minor);
> > > -
> > >  /* drm_vblank.c */
> > >  void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe);
> > >  void drm_vblank_cleanup(struct drm_device *dev);
> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > index 68ca736c548d..24f8d054c570 100644
> > > --- a/include/drm/drm_drv.h
> > > +++ b/include/drm/drm_drv.h
> > > @@ -799,5 +799,9 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
> > >
> > >  int drm_dev_set_unique(struct drm_device *dev, const char *name);
> > >
> > > +int drm_minor_for_each(int (*fn)(int id, void *p, void *data), void *data);
> > > +
> > > +struct drm_minor *drm_minor_acquire(unsigned int minor_id);
> > > +void drm_minor_release(struct drm_minor *minor);
> > >
> > >  #endif
> > > --
> > > 2.22.0
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx