Re: [PATCH 03/30] drm/tegra: Don't register DP AUX channels before connectors

[Lyude Paul]

On Wed, 2021-04-14 at 18:49 +0200, Thierry Reding wrote:
> On Fri, Feb 19, 2021 at 04:52:59PM -0500, Lyude Paul wrote:
> > As pointed out by the documentation for drm_dp_aux_register(),
> > drm_dp_aux_init() should be used in situations where the AUX channel for a
> > display driver can potentially be registered before it's respective DRM
> > driver. This is the case with Tegra, since the DP aux channel exists as a
> > platform device instead of being a grandchild of the DRM device.
> > 
> > Since we're about to add a backpointer to a DP AUX channel's respective
> > DRM
> > device, let's fix this so that we don't potentially allow userspace to use
> > the AUX channel before we've associated it with it's DRM connector.
> > 
> > Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/tegra/dpaux.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > index 105fb9cdbb3b..ea56c6ec25e4 100644
> > --- a/drivers/gpu/drm/tegra/dpaux.c
> > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > @@ -534,9 +534,7 @@ static int tegra_dpaux_probe(struct platform_device
> > *pdev)
> >         dpaux->aux.transfer = tegra_dpaux_transfer;
> >         dpaux->aux.dev = &pdev->dev;
> >  
> > -       err = drm_dp_aux_register(&dpaux->aux);
> > -       if (err < 0)
> > -               return err;
> > +       drm_dp_aux_init(&dpaux->aux);
> 
> I just noticed that this change causes an error on some setups that I
> haven't seen before. The problem is that the SOR driver tries to grab a
> reference to the I2C device to make sure it doesn't go away while it has
> a pointer to it.
> 
> However, since now the I2C adapter hasn't been registered yet, I get
> this:
> 
>         [   15.013969] kobject: '(null)' (000000005c903e43): is not
> initialized, yet kobject_get() is being called.
> 
> I recall that you wanted to make this change so that a backpointer to
> the DRM device could be added (I think that's patch 15 of the series),
> but I didn't see that patch get merged, so it's a bit difficult to try
> and fix this up.

I'm pretty sure I already merged the tegra change in drm-misc-next, so if it's
causing issues you probably should send out a revert for now and I can r-b it
so we can figure out a better solution for this in the mean time

> Has the situation changed? Do we no longer need the backpointer? If we
> still want it, what's the plan for merging the change? Should I work
> under the assumption that patch will make it in sometime and try to fix
> this on top of that?

yes we do still need the backpointer - I'm just still working on getting
reviews for some of the other parts of this series, and have been on PTO/busy
with a couple of other things.

> 
> I'm thinking that perhaps we can move the I2C adapter registration into
> drm_dp_aux_init() since that's independent of the DRM device.

Yeah this makes sense for me - I can try to make this change on the next
respin of this series. What kind of setup were you able to reproduce issues on
this with btw?

>  It would
> also make a bit more sense from the Tegra driver's point of view where
> all devices would be created during the ->probe() path, and only during
> the ->init() path would the connection between DRM device and DRM DP AUX
> device be established.
> 
> Thierry

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat