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

Kenny Ho y2kenny at gmail.com
Thu Sep 5 20:20:55 UTC 2019 

On Thu, Sep 5, 2019 at 4:06 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Thu, Sep 5, 2019 at 8:28 PM Kenny Ho <y2kenny at gmail.com> wrote:
> >
> > (resent in plain text mode)
> >
> > Hi Daniel,
> >
> > This is the previous patch relevant to this discussion:
> > https://patchwork.freedesktop.org/patch/314343/
>
> Ah yes, thanks for finding that.
>
> > So before I refactored the code to leverage drm_minor, I kept my own
> > list of "known" drm_device inside the controller and have explicit
> > register and unregister function to init per device cgroup defaults.
> > For v4, I refactored the per device cgroup properties and embedded
> > them into the drm_device and continue to only use the primary minor as
> > a way to index the device as v3.
>
> I didn't really like the explicit registration step, at least for the
> basic cgroup controls (like gem buffer limits), and suggested that
> should happen automatically at drm_dev_register/unregister time. I
> also talked about picking a consistent minor (if we have to use
> minors, still would like Tejun to confirm what we should do here), but
> that was an unrelated comment. So doing auto-registration on drm_minor
> was one step too far.

How about your comments on embedding properties into drm_device?  I am
actually still not clear on the downside of using drm_minor this way.
With this implementation in v4, there isn't additional state that can
go out of sync with the ground truth of drm_device from the
perspective of drm_minor.  Wouldn't the issue with hotplugging drm
device you described earlier get worsen if the cgroup controller keep
its own list?

> Just doing a drm_cg_register/unregister pair that's called from
> drm_dev_register/unregister, and then if you want, looking up the
> right minor (I think always picking the render node makes sense for
> this, and skipping if there's no render node) would make most sense.
> At least for the basic cgroup controllers which are generic across
> drivers.

Why do we want to skip drm devices that does not have a render node
and not just use the primary instead?

Kenny



> -Daniel
>
>
>
> >
> > Regards,
> > Kenny
> >
> >
> > On Wed, Sep 4, 2019 at 4:54 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Tue, Sep 03, 2019 at 04:43:45PM -0400, Kenny Ho wrote:
> > > > On Tue, Sep 3, 2019 at 4:12 PM Daniel Vetter <daniel at ffwll.ch> wrote:
> > > > > On Tue, Sep 3, 2019 at 9:45 PM Kenny Ho <y2kenny at gmail.com> wrote:
> > > > > > On Tue, Sep 3, 2019 at 3:57 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> > > > > > > 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.
> > > >
> > > > Um... I thought this is what I had previously.  Did I misunderstood
> > > > your feedback from v3?  Doesn't drm_minor already include all these
> > > > facilities so isn't creating my own kind of reinventing the wheel?
> > > > (as I did previously?)  drm_minor_register is called inside
> > > > drm_dev_register so isn't leveraging existing drm_minor facilities
> > > > much better solution?
> > >
> > > Hm the previous version already dropped out of my inbox, so hard to find
> > > it again. And I couldn't find this in archieves. Do you have pointers?
> > >
> > > I thought the previous version did cgroup init separately from drm_device
> > > setup, and I guess I suggested that it should be moved int
> > > drm_dev_register/unregister?
> > >
> > > Anyway, I don't think reusing the drm_minor registration makes sense,
> > > since we want to be on the drm_device, not on the minor. Which is a bit
> > > awkward for cgroups, which wants to identify devices using major.minor
> > > pairs. But I guess drm is the first subsystem where 1 device can be
> > > exposed through multiple minors ...
> > >
> > > Tejun, any suggestions on this?
> > >
> > > Anyway, I think just leveraging existing code because it can be abused to
> > > make it fit for us doesn't make sense. E.g. for the kms side we also don't
> > > piggy-back on top of drm_minor_register (it would be technically
> > > possible), but instead we have drm_modeset_register_all().
> > > -Daniel
> > >
> > > >
> > > > Kenny
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > 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
> > >
> > > --
> > > 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

More information about the amd-gfx mailing list