[RFC 8/9] drm/client: Add drm_client_init_from_id() and drm_client_modeset_set()

[Daniel Vetter]

On Sun, Feb 23, 2020 at 6:43 PM Noralf Trønnes <noralf at tronnes.org> wrote:
> Den 17.02.2020 10.38, skrev Daniel Vetter:
> > On Sun, Feb 16, 2020 at 06:21:16PM +0100, Noralf Trønnes wrote:
> >> drm_client_init_from_id() provides a way for clients to add a client based
> >> on the minor. drm_client_modeset_set() provides a way to set the modeset
> >> for clients that handles connectors and display mode on their own.
> >>
> >> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> >> ---
> >>  drivers/gpu/drm/drm_client.c         | 37 ++++++++++++++++++++
> >>  drivers/gpu/drm/drm_client_modeset.c | 52 ++++++++++++++++++++++++++++
> >>  include/drm/drm_client.h             |  4 +++
> >>  3 files changed, 93 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> >> index d9a2e3695525..dbd73fe8d987 100644
> >> --- a/drivers/gpu/drm/drm_client.c
> >> +++ b/drivers/gpu/drm/drm_client.c
> >> @@ -112,6 +112,43 @@ int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
> >>  }
> >>  EXPORT_SYMBOL(drm_client_init);
> >>
> >> +/**
> >> + * drm_client_init_from_id - Initialise a DRM client
> >> + * @minor_id: DRM minor id
> >> + * @client: DRM client
> >> + * @name: Client name
> >> + * @funcs: DRM client functions (optional)
> >> + *
> >> + * This function looks up the drm_device using the minor id and initializes the client.
> >> + * It also registeres the client to avoid a possible race with DRM device unregister.
> >
> > I think another sentence here would be good, explaining that this is for
> > drivers outside of drm to expose a specific drm driver in some fashion,
> > and just outright mention your use-case as an example.
> >
> > I'm also not sure whether lookup-by-minor is the greatest thing for
> > kernel-internal lookups like this, maybe Greg has some idea?
> >
>
> What alternatives do you see? Parent device name?
>
> I did a scan to see what others are doing, and most have a consumer name
> lookup on the struct device (Device Tree or lookup tables):

I think those are all for other purposes. What we want here is that
some interface things binds to something else. I was thinking sysfs
paths to the underlying struct device (i.e. the one in
drm_device->dev) might be neater, since that's more unique. Using
minors we get that entire trouble of having duplicates (and in the
past triplicatres) due to our uapi-flavors. But in sysfs there's only
one underlying device.

Anyway if there's no precedence and Greg doesn't have ideas either
then I guess we can just go with minor ids. It's about as good as
anything else really.
-Daniel

>
> struct reset_control *__reset_control_get(struct device *dev, const char
> *id,
>                                           int index, bool shared,
>                                           bool optional, bool acquired);
>
> struct iio_channel *iio_channel_get(struct device *dev,
>                                     const char *consumer_channel);
>
> struct regulator *__must_check regulator_get(struct device *dev,
>                                              const char *id);
>
> struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>
> struct gpio_desc *__must_check gpiod_get(struct device *dev,
>                                          const char *con_id,
>                                          enum gpiod_flags flags);
>
> SPI and I2C use the bus index as lookup:
>
> extern struct i2c_adapter *i2c_get_adapter(int nr);
>
> extern struct spi_controller *spi_busnum_to_master(u16 busnum);
>
>
> Noralf.
>
> >> + *
> >> + * See drm_client_init() and drm_client_register().
> >> + *
> >> + * Returns:
> >> + * Zero on success or negative error code on failure.
> >> + */
> >> +int drm_client_init_from_id(unsigned int minor_id, struct drm_client_dev *client,
> >> +                        const char *name, const struct drm_client_funcs *funcs)
> >> +{
> >> +    struct drm_minor *minor;
> >> +    int ret;
> >> +
> >> +    minor = drm_minor_acquire(minor_id);
> >> +    if (IS_ERR(minor))
> >> +            return PTR_ERR(minor);
> >> +
> >> +    mutex_lock(&minor->dev->clientlist_mutex);
> >> +    ret = drm_client_init(minor->dev, client, name, funcs);
> >> +    if (!ret)
> >> +            list_add(&client->list, &minor->dev->clientlist);
> >> +    mutex_unlock(&minor->dev->clientlist_mutex);
> >> +
> >> +    drm_minor_release(minor);
> >> +
> >> +    return ret;
> >> +}
> >> +EXPORT_SYMBOL(drm_client_init_from_id);
> >> +
> >>  /**
> >>   * drm_client_register - Register client
> >>   * @client: DRM client



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch