Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <0a64b70c89699b4e6c7aaa3ba8e75531031faa3c.camel@redhat.com>
Subject: Re: [PATCH 03/30] drm/tegra: Don't register DP AUX channels before
 connectors
From:   Lyude Paul <lyude@redhat.com>
To:     Thierry Reding <thierry.reding@gmail.com>
Cc:     intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        amd-gfx@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= 
        <ville.syrjala@linux.intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        "open list:DRM DRIVERS FOR NVIDIA TEGRA" 
        <linux-tegra@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Date:   Wed, 14 Apr 2021 14:17:45 -0400
In-Reply-To: <YHcdCAQ7Dlb2vSzn@orome.fritz.box>
References: <20210219215326.2227596-1-lyude@redhat.com>
         <20210219215326.2227596-4-lyude@redhat.com>
         <YHcdCAQ7Dlb2vSzn@orome.fritz.box>
Organization: Red Hat
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@redhat.com>
> > ---
> >  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