[v6,2/5] drm/dp: Allow an early call to register DDC i2c bus

Message ID 20210503145750.v6.2.Iff8f2957d86af40f2bfcfb5a7163928481fccea4@changeid
State New
Headers show
Series
  • drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems
Related show

Commit Message

Doug Anderson May 3, 2021, 9:58 p.m.
It can be helpful to fully register the AUX channel as an i2c bus even
before the bridge is created. Let's optionally allow bridges to do
that.

Specifically the case we're running into:
- The panel driver wants to get its DDC bus at probe time.
- The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
  bus, wants to get the panel at probe time.

The next patches ("drm/bridge: ti-sn65dsi86: Promote the AUX channel
to its own sub-dev") solves the chicken-and-egg problem by breaking
the ti-sn65dsi86 driver into sub-devices, but in order for it to
actually work we need the i2c bus to get registered at probe time and
not in bridge attach time.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v6:
- ("drm/dp: Allow an early call to register DDC i2c bus") new for v6.

 drivers/gpu/drm/drm_dp_helper.c | 67 +++++++++++++++++++++++++++------
 include/drm/drm_dp_helper.h     |  2 +
 2 files changed, 57 insertions(+), 12 deletions(-)

Comments

Lyude Paul May 7, 2021, 9:18 p.m. | #1
Adding ville from Intel to also get their take on this.

In general we've been trying to move DRM to a design where we don't expose any
devices until everything is ready. That's pretty much the main reason that we
register things during bridge attach time. Note though that even before the
DDC bus is registered it should still be usable, just things like get_device()
won't work.

This isn't the first time we've run into a problem like the one you're trying
to solve though, Tegra currently has a similar issue. Something we discussed
as a possible long-term solution for this was splitting i2c_add_adapter() into
a minimal initialization function and a registration function. Linux's device
core already allows for this (device_initialize() and device_add(), which are
called together when device_register() is called). Would this be a solution
that might work for you (and even better, would you possibly be willing to
write the patches? :)

On Mon, 2021-05-03 at 14:58 -0700, Douglas Anderson wrote:
> It can be helpful to fully register the AUX channel as an i2c bus even
> before the bridge is created. Let's optionally allow bridges to do
> that.
> 
> Specifically the case we're running into:
> - The panel driver wants to get its DDC bus at probe time.
> - The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
>   bus, wants to get the panel at probe time.
> 
> The next patches ("drm/bridge: ti-sn65dsi86: Promote the AUX channel
> to its own sub-dev") solves the chicken-and-egg problem by breaking
> the ti-sn65dsi86 driver into sub-devices, but in order for it to
> actually work we need the i2c bus to get registered at probe time and
> not in bridge attach time.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v6:
> - ("drm/dp: Allow an early call to register DDC i2c bus") new for v6.
> 
>  drivers/gpu/drm/drm_dp_helper.c | 67 +++++++++++++++++++++++++++------
>  include/drm/drm_dp_helper.h     |  2 +
>  2 files changed, 57 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index cb56d74e9d38..830294f0b341 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1757,6 +1757,49 @@ void drm_dp_aux_init(struct drm_dp_aux *aux)
>  }
>  EXPORT_SYMBOL(drm_dp_aux_init);
>  
> +/**
> + * drm_dp_aux_register_ddc() - register the DDC parts of the aux channel
> + * @aux: DisplayPort AUX channel
> + *
> + * This can be called after drm_dp_aux_init() to fully register the ddc bus
> + * as an i2c adapter with the rest of Linux.
> + *
> + * If you don't explicitly call this function it will be done implicitly as
> + * part of drm_dp_aux_register().
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux)
> +{
> +       WARN_ON_ONCE(!aux->dev);
> +
> +       aux->ddc.class = I2C_CLASS_DDC;
> +       aux->ddc.owner = THIS_MODULE;
> +       aux->ddc.dev.parent = aux->dev;
> +
> +       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> +               sizeof(aux->ddc.name));
> +
> +       return i2c_add_adapter(&aux->ddc);
> +}
> +EXPORT_SYMBOL(drm_dp_aux_register_ddc);
> +
> +/**
> + * drm_dp_aux_unregister_ddc() - unregister the DDC parts of the aux
> channel
> + *
> + * This is useful if you called drm_dp_aux_register_ddc(). If you let
> + * drm_dp_aux_register() implicitly register the DDC for you then you don't
> + * need to worry about calling this yourself.
> + *
> + * @aux: DisplayPort AUX channel
> + */
> +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux)
> +{
> +       i2c_del_adapter(&aux->ddc);
> +       aux->ddc.dev.parent = NULL;
> +}
> +EXPORT_SYMBOL(drm_dp_aux_unregister_ddc);
> +
>  /**
>   * drm_dp_aux_register() - initialise and register aux channel
>   * @aux: DisplayPort AUX channel
> @@ -1793,20 +1836,19 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>         if (!aux->ddc.algo)
>                 drm_dp_aux_init(aux);
>  
> -       aux->ddc.class = I2C_CLASS_DDC;
> -       aux->ddc.owner = THIS_MODULE;
> -       aux->ddc.dev.parent = aux->dev;
> -
> -       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> -               sizeof(aux->ddc.name));
> +       /*
> +        * Implicitly register if drm_dp_aux_register_ddc() wasn't already
> +        * called (as evidenced by a NULL parent pointer).
> +        */
> +       if (!aux->ddc.dev.parent) {
> +               ret = drm_dp_aux_register_ddc(aux);
> +               if (ret)
> +                       return ret;
> +       }
>  
>         ret = drm_dp_aux_register_devnode(aux);
> -       if (ret)
> -               return ret;
> -
> -       ret = i2c_add_adapter(&aux->ddc);
>         if (ret) {
> -               drm_dp_aux_unregister_devnode(aux);
> +               drm_dp_aux_unregister_ddc(aux);
>                 return ret;
>         }
>  
> @@ -1821,7 +1863,8 @@ EXPORT_SYMBOL(drm_dp_aux_register);
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
>  {
>         drm_dp_aux_unregister_devnode(aux);
> -       i2c_del_adapter(&aux->ddc);
> +       if (aux->ddc.dev.parent)
> +               drm_dp_aux_unregister_ddc(aux);
>  }
>  EXPORT_SYMBOL(drm_dp_aux_unregister);
>  
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index e932b2c40095..d4d2d5e25bb7 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -2021,6 +2021,8 @@ bool drm_dp_lttpr_pre_emphasis_level_3_supported(const
> u8 caps[DP_LTTPR_PHY_CAP_
>  
>  void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
>  void drm_dp_aux_init(struct drm_dp_aux *aux);
> +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux);
> +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux);
>  int drm_dp_aux_register(struct drm_dp_aux *aux);
>  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>
Bjorn Andersson May 7, 2021, 10 p.m. | #2
On Fri 07 May 16:18 CDT 2021, Lyude Paul wrote:

> Adding ville from Intel to also get their take on this.
> 
> In general we've been trying to move DRM to a design where we don't expose any
> devices until everything is ready. That's pretty much the main reason that we
> register things during bridge attach time. Note though that even before the
> DDC bus is registered it should still be usable, just things like get_device()
> won't work.
> 
> This isn't the first time we've run into a problem like the one you're trying
> to solve though, Tegra currently has a similar issue. Something we discussed
> as a possible long-term solution for this was splitting i2c_add_adapter() into
> a minimal initialization function and a registration function. Linux's device
> core already allows for this (device_initialize() and device_add(), which are
> called together when device_register() is called). Would this be a solution
> that might work for you (and even better, would you possibly be willing to
> write the patches? :)
> 

It's not enough that the adapter is half-baked, because the bridge's
initialization depends on that the panel device is done probing, and the
panel driver will only complete its probe if it can find it's resources.

So we need a mechanism to fully create the resources exposed by the
bridge chip (i2c bus, gpio chip and (soon) a pwm chip), then allow the
panel to probe and after that initialize the bridge.

We did discuss possible ways to register these resources and then
"sleep for a while" before resolving the panel, but what we came up with
was definitely suboptimal - and ugly.

Regards,
Bjorn

> On Mon, 2021-05-03 at 14:58 -0700, Douglas Anderson wrote:
> > It can be helpful to fully register the AUX channel as an i2c bus even
> > before the bridge is created. Let's optionally allow bridges to do
> > that.
> > 
> > Specifically the case we're running into:
> > - The panel driver wants to get its DDC bus at probe time.
> > - The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
> >   bus, wants to get the panel at probe time.
> > 
> > The next patches ("drm/bridge: ti-sn65dsi86: Promote the AUX channel
> > to its own sub-dev") solves the chicken-and-egg problem by breaking
> > the ti-sn65dsi86 driver into sub-devices, but in order for it to
> > actually work we need the i2c bus to get registered at probe time and
> > not in bridge attach time.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > 
> > Changes in v6:
> > - ("drm/dp: Allow an early call to register DDC i2c bus") new for v6.
> > 
> >  drivers/gpu/drm/drm_dp_helper.c | 67 +++++++++++++++++++++++++++------
> >  include/drm/drm_dp_helper.h     |  2 +
> >  2 files changed, 57 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index cb56d74e9d38..830294f0b341 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -1757,6 +1757,49 @@ void drm_dp_aux_init(struct drm_dp_aux *aux)
> >  }
> >  EXPORT_SYMBOL(drm_dp_aux_init);
> >  
> > +/**
> > + * drm_dp_aux_register_ddc() - register the DDC parts of the aux channel
> > + * @aux: DisplayPort AUX channel
> > + *
> > + * This can be called after drm_dp_aux_init() to fully register the ddc bus
> > + * as an i2c adapter with the rest of Linux.
> > + *
> > + * If you don't explicitly call this function it will be done implicitly as
> > + * part of drm_dp_aux_register().
> > + *
> > + * Returns 0 on success or a negative error code on failure.
> > + */
> > +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux)
> > +{
> > +       WARN_ON_ONCE(!aux->dev);
> > +
> > +       aux->ddc.class = I2C_CLASS_DDC;
> > +       aux->ddc.owner = THIS_MODULE;
> > +       aux->ddc.dev.parent = aux->dev;
> > +
> > +       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> > +               sizeof(aux->ddc.name));
> > +
> > +       return i2c_add_adapter(&aux->ddc);
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_register_ddc);
> > +
> > +/**
> > + * drm_dp_aux_unregister_ddc() - unregister the DDC parts of the aux
> > channel
> > + *
> > + * This is useful if you called drm_dp_aux_register_ddc(). If you let
> > + * drm_dp_aux_register() implicitly register the DDC for you then you don't
> > + * need to worry about calling this yourself.
> > + *
> > + * @aux: DisplayPort AUX channel
> > + */
> > +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux)
> > +{
> > +       i2c_del_adapter(&aux->ddc);
> > +       aux->ddc.dev.parent = NULL;
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_unregister_ddc);
> > +
> >  /**
> >   * drm_dp_aux_register() - initialise and register aux channel
> >   * @aux: DisplayPort AUX channel
> > @@ -1793,20 +1836,19 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> >         if (!aux->ddc.algo)
> >                 drm_dp_aux_init(aux);
> >  
> > -       aux->ddc.class = I2C_CLASS_DDC;
> > -       aux->ddc.owner = THIS_MODULE;
> > -       aux->ddc.dev.parent = aux->dev;
> > -
> > -       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> > -               sizeof(aux->ddc.name));
> > +       /*
> > +        * Implicitly register if drm_dp_aux_register_ddc() wasn't already
> > +        * called (as evidenced by a NULL parent pointer).
> > +        */
> > +       if (!aux->ddc.dev.parent) {
> > +               ret = drm_dp_aux_register_ddc(aux);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >  
> >         ret = drm_dp_aux_register_devnode(aux);
> > -       if (ret)
> > -               return ret;
> > -
> > -       ret = i2c_add_adapter(&aux->ddc);
> >         if (ret) {
> > -               drm_dp_aux_unregister_devnode(aux);
> > +               drm_dp_aux_unregister_ddc(aux);
> >                 return ret;
> >         }
> >  
> > @@ -1821,7 +1863,8 @@ EXPORT_SYMBOL(drm_dp_aux_register);
> >  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> >  {
> >         drm_dp_aux_unregister_devnode(aux);
> > -       i2c_del_adapter(&aux->ddc);
> > +       if (aux->ddc.dev.parent)
> > +               drm_dp_aux_unregister_ddc(aux);
> >  }
> >  EXPORT_SYMBOL(drm_dp_aux_unregister);
> >  
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index e932b2c40095..d4d2d5e25bb7 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -2021,6 +2021,8 @@ bool drm_dp_lttpr_pre_emphasis_level_3_supported(const
> > u8 caps[DP_LTTPR_PHY_CAP_
> >  
> >  void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
> >  void drm_dp_aux_init(struct drm_dp_aux *aux);
> > +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux);
> > +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux);
> >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> >  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
> >  
> 
> -- 
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
>
Lyude Paul May 7, 2021, 10:09 p.m. | #3
On Fri, 2021-05-07 at 17:00 -0500, Bjorn Andersson wrote:
> On Fri 07 May 16:18 CDT 2021, Lyude Paul wrote:
> 
> > Adding ville from Intel to also get their take on this.
> > 
> > In general we've been trying to move DRM to a design where we don't expose
> > any
> > devices until everything is ready. That's pretty much the main reason that
> > we
> > register things during bridge attach time. Note though that even before
> > the
> > DDC bus is registered it should still be usable, just things like
> > get_device()
> > won't work.
> > 
> > This isn't the first time we've run into a problem like the one you're
> > trying
> > to solve though, Tegra currently has a similar issue. Something we
> > discussed
> > as a possible long-term solution for this was splitting i2c_add_adapter()
> > into
> > a minimal initialization function and a registration function. Linux's
> > device
> > core already allows for this (device_initialize() and device_add(), which
> > are
> > called together when device_register() is called). Would this be a
> > solution
> > that might work for you (and even better, would you possibly be willing to
> > write the patches? :)
> > 
> 
> It's not enough that the adapter is half-baked, because the bridge's
> initialization depends on that the panel device is done probing, and the
> panel driver will only complete its probe if it can find it's resources.
> 
> So we need a mechanism to fully create the resources exposed by the
> bridge chip (i2c bus, gpio chip and (soon) a pwm chip), then allow the
> panel to probe and after that initialize the bridge.
> 
> We did discuss possible ways to register these resources and then
> "sleep for a while" before resolving the panel, but what we came up with
> was definitely suboptimal - and ugly.

Sigh, I'm really starting to wonder if we should reconsider the rules on
exposing ddc adapters early...

Danvet, Jani, and/or airlied: can I get your take on this?

> 
> Regards,
> Bjorn
> 
> > On Mon, 2021-05-03 at 14:58 -0700, Douglas Anderson wrote:
> > > It can be helpful to fully register the AUX channel as an i2c bus even
> > > before the bridge is created. Let's optionally allow bridges to do
> > > that.
> > > 
> > > Specifically the case we're running into:
> > > - The panel driver wants to get its DDC bus at probe time.
> > > - The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
> > >   bus, wants to get the panel at probe time.
> > > 
> > > The next patches ("drm/bridge: ti-sn65dsi86: Promote the AUX channel
> > > to its own sub-dev") solves the chicken-and-egg problem by breaking
> > > the ti-sn65dsi86 driver into sub-devices, but in order for it to
> > > actually work we need the i2c bus to get registered at probe time and
> > > not in bridge attach time.
> > > 
> > > Cc: Lyude Paul <lyude@redhat.com>
> > > Cc: Thierry Reding <treding@nvidia.com>
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > 
> > > Changes in v6:
> > > - ("drm/dp: Allow an early call to register DDC i2c bus") new for v6.
> > > 
> > >  drivers/gpu/drm/drm_dp_helper.c | 67 +++++++++++++++++++++++++++------
> > >  include/drm/drm_dp_helper.h     |  2 +
> > >  2 files changed, 57 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > > b/drivers/gpu/drm/drm_dp_helper.c
> > > index cb56d74e9d38..830294f0b341 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -1757,6 +1757,49 @@ void drm_dp_aux_init(struct drm_dp_aux *aux)
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_aux_init);
> > >  
> > > +/**
> > > + * drm_dp_aux_register_ddc() - register the DDC parts of the aux
> > > channel
> > > + * @aux: DisplayPort AUX channel
> > > + *
> > > + * This can be called after drm_dp_aux_init() to fully register the ddc
> > > bus
> > > + * as an i2c adapter with the rest of Linux.
> > > + *
> > > + * If you don't explicitly call this function it will be done
> > > implicitly as
> > > + * part of drm_dp_aux_register().
> > > + *
> > > + * Returns 0 on success or a negative error code on failure.
> > > + */
> > > +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux)
> > > +{
> > > +       WARN_ON_ONCE(!aux->dev);
> > > +
> > > +       aux->ddc.class = I2C_CLASS_DDC;
> > > +       aux->ddc.owner = THIS_MODULE;
> > > +       aux->ddc.dev.parent = aux->dev;
> > > +
> > > +       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux-
> > > >dev),
> > > +               sizeof(aux->ddc.name));
> > > +
> > > +       return i2c_add_adapter(&aux->ddc);
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_aux_register_ddc);
> > > +
> > > +/**
> > > + * drm_dp_aux_unregister_ddc() - unregister the DDC parts of the aux
> > > channel
> > > + *
> > > + * This is useful if you called drm_dp_aux_register_ddc(). If you let
> > > + * drm_dp_aux_register() implicitly register the DDC for you then you
> > > don't
> > > + * need to worry about calling this yourself.
> > > + *
> > > + * @aux: DisplayPort AUX channel
> > > + */
> > > +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux)
> > > +{
> > > +       i2c_del_adapter(&aux->ddc);
> > > +       aux->ddc.dev.parent = NULL;
> > > +}
> > > +EXPORT_SYMBOL(drm_dp_aux_unregister_ddc);
> > > +
> > >  /**
> > >   * drm_dp_aux_register() - initialise and register aux channel
> > >   * @aux: DisplayPort AUX channel
> > > @@ -1793,20 +1836,19 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> > >         if (!aux->ddc.algo)
> > >                 drm_dp_aux_init(aux);
> > >  
> > > -       aux->ddc.class = I2C_CLASS_DDC;
> > > -       aux->ddc.owner = THIS_MODULE;
> > > -       aux->ddc.dev.parent = aux->dev;
> > > -
> > > -       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux-
> > > >dev),
> > > -               sizeof(aux->ddc.name));
> > > +       /*
> > > +        * Implicitly register if drm_dp_aux_register_ddc() wasn't
> > > already
> > > +        * called (as evidenced by a NULL parent pointer).
> > > +        */
> > > +       if (!aux->ddc.dev.parent) {
> > > +               ret = drm_dp_aux_register_ddc(aux);
> > > +               if (ret)
> > > +                       return ret;
> > > +       }
> > >  
> > >         ret = drm_dp_aux_register_devnode(aux);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > > -       ret = i2c_add_adapter(&aux->ddc);
> > >         if (ret) {
> > > -               drm_dp_aux_unregister_devnode(aux);
> > > +               drm_dp_aux_unregister_ddc(aux);
> > >                 return ret;
> > >         }
> > >  
> > > @@ -1821,7 +1863,8 @@ EXPORT_SYMBOL(drm_dp_aux_register);
> > >  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> > >  {
> > >         drm_dp_aux_unregister_devnode(aux);
> > > -       i2c_del_adapter(&aux->ddc);
> > > +       if (aux->ddc.dev.parent)
> > > +               drm_dp_aux_unregister_ddc(aux);
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_aux_unregister);
> > >  
> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > > index e932b2c40095..d4d2d5e25bb7 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -2021,6 +2021,8 @@ bool
> > > drm_dp_lttpr_pre_emphasis_level_3_supported(const
> > > u8 caps[DP_LTTPR_PHY_CAP_
> > >  
> > >  void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
> > >  void drm_dp_aux_init(struct drm_dp_aux *aux);
> > > +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux);
> > > +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux);
> > >  int drm_dp_aux_register(struct drm_dp_aux *aux);
> > >  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
> > >  
> > 
> > -- 
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> > 
>
Jani Nikula May 14, 2021, 11:16 a.m. | #4
On Fri, 07 May 2021, Lyude Paul <lyude@redhat.com> wrote:
> On Fri, 2021-05-07 at 17:00 -0500, Bjorn Andersson wrote:
>> On Fri 07 May 16:18 CDT 2021, Lyude Paul wrote:
>> 
>> > Adding ville from Intel to also get their take on this.
>> > 
>> > In general we've been trying to move DRM to a design where we don't expose
>> > any
>> > devices until everything is ready. That's pretty much the main reason that
>> > we
>> > register things during bridge attach time. Note though that even before
>> > the
>> > DDC bus is registered it should still be usable, just things like
>> > get_device()
>> > won't work.
>> > 
>> > This isn't the first time we've run into a problem like the one you're
>> > trying
>> > to solve though, Tegra currently has a similar issue. Something we
>> > discussed
>> > as a possible long-term solution for this was splitting i2c_add_adapter()
>> > into
>> > a minimal initialization function and a registration function. Linux's
>> > device
>> > core already allows for this (device_initialize() and device_add(), which
>> > are
>> > called together when device_register() is called). Would this be a
>> > solution
>> > that might work for you (and even better, would you possibly be willing to
>> > write the patches? :)
>> > 
>> 
>> It's not enough that the adapter is half-baked, because the bridge's
>> initialization depends on that the panel device is done probing, and the
>> panel driver will only complete its probe if it can find it's resources.
>> 
>> So we need a mechanism to fully create the resources exposed by the
>> bridge chip (i2c bus, gpio chip and (soon) a pwm chip), then allow the
>> panel to probe and after that initialize the bridge.
>> 
>> We did discuss possible ways to register these resources and then
>> "sleep for a while" before resolving the panel, but what we came up with
>> was definitely suboptimal - and ugly.
>
> Sigh, I'm really starting to wonder if we should reconsider the rules on
> exposing ddc adapters early...
>
> Danvet, Jani, and/or airlied: can I get your take on this?

Granted, I did not study this in detail, but it sounds like we'd need to
be able to add and use an i2c adapter in kernel, before deciding to
register it with the userspace. But that does not seem to be as trivial
as making it possible to call the now-static i2c_register_adapter()
separately.


BR,
Jani.


>
>> 
>> Regards,
>> Bjorn
>> 
>> > On Mon, 2021-05-03 at 14:58 -0700, Douglas Anderson wrote:
>> > > It can be helpful to fully register the AUX channel as an i2c bus even
>> > > before the bridge is created. Let's optionally allow bridges to do
>> > > that.
>> > > 
>> > > Specifically the case we're running into:
>> > > - The panel driver wants to get its DDC bus at probe time.
>> > > - The ti-sn65dsi86 MIPI-to-eDP bridge code, which provides the DDC
>> > >   bus, wants to get the panel at probe time.
>> > > 
>> > > The next patches ("drm/bridge: ti-sn65dsi86: Promote the AUX channel
>> > > to its own sub-dev") solves the chicken-and-egg problem by breaking
>> > > the ti-sn65dsi86 driver into sub-devices, but in order for it to
>> > > actually work we need the i2c bus to get registered at probe time and
>> > > not in bridge attach time.
>> > > 
>> > > Cc: Lyude Paul <lyude@redhat.com>
>> > > Cc: Thierry Reding <treding@nvidia.com>
>> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> > > ---
>> > > 
>> > > Changes in v6:
>> > > - ("drm/dp: Allow an early call to register DDC i2c bus") new for v6.
>> > > 
>> > >  drivers/gpu/drm/drm_dp_helper.c | 67 +++++++++++++++++++++++++++------
>> > >  include/drm/drm_dp_helper.h     |  2 +
>> > >  2 files changed, 57 insertions(+), 12 deletions(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> > > b/drivers/gpu/drm/drm_dp_helper.c
>> > > index cb56d74e9d38..830294f0b341 100644
>> > > --- a/drivers/gpu/drm/drm_dp_helper.c
>> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
>> > > @@ -1757,6 +1757,49 @@ void drm_dp_aux_init(struct drm_dp_aux *aux)
>> > >  }
>> > >  EXPORT_SYMBOL(drm_dp_aux_init);
>> > >  
>> > > +/**
>> > > + * drm_dp_aux_register_ddc() - register the DDC parts of the aux
>> > > channel
>> > > + * @aux: DisplayPort AUX channel
>> > > + *
>> > > + * This can be called after drm_dp_aux_init() to fully register the ddc
>> > > bus
>> > > + * as an i2c adapter with the rest of Linux.
>> > > + *
>> > > + * If you don't explicitly call this function it will be done
>> > > implicitly as
>> > > + * part of drm_dp_aux_register().
>> > > + *
>> > > + * Returns 0 on success or a negative error code on failure.
>> > > + */
>> > > +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux)
>> > > +{
>> > > +       WARN_ON_ONCE(!aux->dev);
>> > > +
>> > > +       aux->ddc.class = I2C_CLASS_DDC;
>> > > +       aux->ddc.owner = THIS_MODULE;
>> > > +       aux->ddc.dev.parent = aux->dev;
>> > > +
>> > > +       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux-
>> > > >dev),
>> > > +               sizeof(aux->ddc.name));
>> > > +
>> > > +       return i2c_add_adapter(&aux->ddc);
>> > > +}
>> > > +EXPORT_SYMBOL(drm_dp_aux_register_ddc);
>> > > +
>> > > +/**
>> > > + * drm_dp_aux_unregister_ddc() - unregister the DDC parts of the aux
>> > > channel
>> > > + *
>> > > + * This is useful if you called drm_dp_aux_register_ddc(). If you let
>> > > + * drm_dp_aux_register() implicitly register the DDC for you then you
>> > > don't
>> > > + * need to worry about calling this yourself.
>> > > + *
>> > > + * @aux: DisplayPort AUX channel
>> > > + */
>> > > +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux)
>> > > +{
>> > > +       i2c_del_adapter(&aux->ddc);
>> > > +       aux->ddc.dev.parent = NULL;
>> > > +}
>> > > +EXPORT_SYMBOL(drm_dp_aux_unregister_ddc);
>> > > +
>> > >  /**
>> > >   * drm_dp_aux_register() - initialise and register aux channel
>> > >   * @aux: DisplayPort AUX channel
>> > > @@ -1793,20 +1836,19 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
>> > >         if (!aux->ddc.algo)
>> > >                 drm_dp_aux_init(aux);
>> > >  
>> > > -       aux->ddc.class = I2C_CLASS_DDC;
>> > > -       aux->ddc.owner = THIS_MODULE;
>> > > -       aux->ddc.dev.parent = aux->dev;
>> > > -
>> > > -       strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux-
>> > > >dev),
>> > > -               sizeof(aux->ddc.name));
>> > > +       /*
>> > > +        * Implicitly register if drm_dp_aux_register_ddc() wasn't
>> > > already
>> > > +        * called (as evidenced by a NULL parent pointer).
>> > > +        */
>> > > +       if (!aux->ddc.dev.parent) {
>> > > +               ret = drm_dp_aux_register_ddc(aux);
>> > > +               if (ret)
>> > > +                       return ret;
>> > > +       }
>> > >  
>> > >         ret = drm_dp_aux_register_devnode(aux);
>> > > -       if (ret)
>> > > -               return ret;
>> > > -
>> > > -       ret = i2c_add_adapter(&aux->ddc);
>> > >         if (ret) {
>> > > -               drm_dp_aux_unregister_devnode(aux);
>> > > +               drm_dp_aux_unregister_ddc(aux);
>> > >                 return ret;
>> > >         }
>> > >  
>> > > @@ -1821,7 +1863,8 @@ EXPORT_SYMBOL(drm_dp_aux_register);
>> > >  void drm_dp_aux_unregister(struct drm_dp_aux *aux)
>> > >  {
>> > >         drm_dp_aux_unregister_devnode(aux);
>> > > -       i2c_del_adapter(&aux->ddc);
>> > > +       if (aux->ddc.dev.parent)
>> > > +               drm_dp_aux_unregister_ddc(aux);
>> > >  }
>> > >  EXPORT_SYMBOL(drm_dp_aux_unregister);
>> > >  
>> > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> > > index e932b2c40095..d4d2d5e25bb7 100644
>> > > --- a/include/drm/drm_dp_helper.h
>> > > +++ b/include/drm/drm_dp_helper.h
>> > > @@ -2021,6 +2021,8 @@ bool
>> > > drm_dp_lttpr_pre_emphasis_level_3_supported(const
>> > > u8 caps[DP_LTTPR_PHY_CAP_
>> > >  
>> > >  void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
>> > >  void drm_dp_aux_init(struct drm_dp_aux *aux);
>> > > +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux);
>> > > +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux);
>> > >  int drm_dp_aux_register(struct drm_dp_aux *aux);
>> > >  void drm_dp_aux_unregister(struct drm_dp_aux *aux);
>> > >  
>> > 
>> > -- 
>> > Cheers,
>> >  Lyude Paul (she/her)
>> >  Software Engineer at Red Hat
>> > 
>>
Doug Anderson May 17, 2021, 8:17 p.m. | #5
Hi,

On Fri, May 14, 2021 at 4:16 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Fri, 07 May 2021, Lyude Paul <lyude@redhat.com> wrote:
> > On Fri, 2021-05-07 at 17:00 -0500, Bjorn Andersson wrote:
> >> On Fri 07 May 16:18 CDT 2021, Lyude Paul wrote:
> >>
> >> > Adding ville from Intel to also get their take on this.
> >> >
> >> > In general we've been trying to move DRM to a design where we don't expose
> >> > any
> >> > devices until everything is ready. That's pretty much the main reason that
> >> > we
> >> > register things during bridge attach time. Note though that even before
> >> > the
> >> > DDC bus is registered it should still be usable, just things like
> >> > get_device()
> >> > won't work.
> >> >
> >> > This isn't the first time we've run into a problem like the one you're
> >> > trying
> >> > to solve though, Tegra currently has a similar issue. Something we
> >> > discussed
> >> > as a possible long-term solution for this was splitting i2c_add_adapter()
> >> > into
> >> > a minimal initialization function and a registration function. Linux's
> >> > device
> >> > core already allows for this (device_initialize() and device_add(), which
> >> > are
> >> > called together when device_register() is called). Would this be a
> >> > solution
> >> > that might work for you (and even better, would you possibly be willing to
> >> > write the patches? :)
> >> >
> >>
> >> It's not enough that the adapter is half-baked, because the bridge's
> >> initialization depends on that the panel device is done probing, and the
> >> panel driver will only complete its probe if it can find it's resources.
> >>
> >> So we need a mechanism to fully create the resources exposed by the
> >> bridge chip (i2c bus, gpio chip and (soon) a pwm chip), then allow the
> >> panel to probe and after that initialize the bridge.
> >>
> >> We did discuss possible ways to register these resources and then
> >> "sleep for a while" before resolving the panel, but what we came up with
> >> was definitely suboptimal - and ugly.
> >
> > Sigh, I'm really starting to wonder if we should reconsider the rules on
> > exposing ddc adapters early...
> >
> > Danvet, Jani, and/or airlied: can I get your take on this?
>
> Granted, I did not study this in detail, but it sounds like we'd need to
> be able to add and use an i2c adapter in kernel, before deciding to
> register it with the userspace. But that does not seem to be as trivial
> as making it possible to call the now-static i2c_register_adapter()
> separately.

To close the loop: I think the point is now moot in v7. Now crossing
my fingers that approach can gain momentum. If not, I might come back
here. ;-)

-Doug

Patch

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index cb56d74e9d38..830294f0b341 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1757,6 +1757,49 @@  void drm_dp_aux_init(struct drm_dp_aux *aux)
 }
 EXPORT_SYMBOL(drm_dp_aux_init);
 
+/**
+ * drm_dp_aux_register_ddc() - register the DDC parts of the aux channel
+ * @aux: DisplayPort AUX channel
+ *
+ * This can be called after drm_dp_aux_init() to fully register the ddc bus
+ * as an i2c adapter with the rest of Linux.
+ *
+ * If you don't explicitly call this function it will be done implicitly as
+ * part of drm_dp_aux_register().
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_aux_register_ddc(struct drm_dp_aux *aux)
+{
+	WARN_ON_ONCE(!aux->dev);
+
+	aux->ddc.class = I2C_CLASS_DDC;
+	aux->ddc.owner = THIS_MODULE;
+	aux->ddc.dev.parent = aux->dev;
+
+	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
+		sizeof(aux->ddc.name));
+
+	return i2c_add_adapter(&aux->ddc);
+}
+EXPORT_SYMBOL(drm_dp_aux_register_ddc);
+
+/**
+ * drm_dp_aux_unregister_ddc() - unregister the DDC parts of the aux channel
+ *
+ * This is useful if you called drm_dp_aux_register_ddc(). If you let
+ * drm_dp_aux_register() implicitly register the DDC for you then you don't
+ * need to worry about calling this yourself.
+ *
+ * @aux: DisplayPort AUX channel
+ */
+void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux)
+{
+	i2c_del_adapter(&aux->ddc);
+	aux->ddc.dev.parent = NULL;
+}
+EXPORT_SYMBOL(drm_dp_aux_unregister_ddc);
+
 /**
  * drm_dp_aux_register() - initialise and register aux channel
  * @aux: DisplayPort AUX channel
@@ -1793,20 +1836,19 @@  int drm_dp_aux_register(struct drm_dp_aux *aux)
 	if (!aux->ddc.algo)
 		drm_dp_aux_init(aux);
 
-	aux->ddc.class = I2C_CLASS_DDC;
-	aux->ddc.owner = THIS_MODULE;
-	aux->ddc.dev.parent = aux->dev;
-
-	strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
-		sizeof(aux->ddc.name));
+	/*
+	 * Implicitly register if drm_dp_aux_register_ddc() wasn't already
+	 * called (as evidenced by a NULL parent pointer).
+	 */
+	if (!aux->ddc.dev.parent) {
+		ret = drm_dp_aux_register_ddc(aux);
+		if (ret)
+			return ret;
+	}
 
 	ret = drm_dp_aux_register_devnode(aux);
-	if (ret)
-		return ret;
-
-	ret = i2c_add_adapter(&aux->ddc);
 	if (ret) {
-		drm_dp_aux_unregister_devnode(aux);
+		drm_dp_aux_unregister_ddc(aux);
 		return ret;
 	}
 
@@ -1821,7 +1863,8 @@  EXPORT_SYMBOL(drm_dp_aux_register);
 void drm_dp_aux_unregister(struct drm_dp_aux *aux)
 {
 	drm_dp_aux_unregister_devnode(aux);
-	i2c_del_adapter(&aux->ddc);
+	if (aux->ddc.dev.parent)
+		drm_dp_aux_unregister_ddc(aux);
 }
 EXPORT_SYMBOL(drm_dp_aux_unregister);
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index e932b2c40095..d4d2d5e25bb7 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -2021,6 +2021,8 @@  bool drm_dp_lttpr_pre_emphasis_level_3_supported(const u8 caps[DP_LTTPR_PHY_CAP_
 
 void drm_dp_remote_aux_init(struct drm_dp_aux *aux);
 void drm_dp_aux_init(struct drm_dp_aux *aux);
+int drm_dp_aux_register_ddc(struct drm_dp_aux *aux);
+void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux);
 int drm_dp_aux_register(struct drm_dp_aux *aux);
 void drm_dp_aux_unregister(struct drm_dp_aux *aux);