Re: Regression with mainline kernel on rpi4

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 28, 2021 at 10:34:46AM +0200, Maxime Ripard wrote:
> Hi Daniel,
> 
> On Sat, Sep 25, 2021 at 12:50:17AM +0200, Daniel Vetter wrote:
> > On Fri, Sep 24, 2021 at 3:30 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Wed, Sep 22, 2021 at 01:25:21PM -0700, Linus Torvalds wrote:
> > > > On Wed, Sep 22, 2021 at 1:19 PM Sudip Mukherjee
> > > > <sudipm.mukherjee@gmail.com> wrote:
> > > > >
> > > > > I added some debugs to print the addresses, and I am getting:
> > > > > [   38.813809] sudip crtc 0000000000000000
> > > > >
> > > > > This is from struct drm_crtc *crtc = connector->state->crtc;
> > > >
> > > > Yeah, that was my personal suspicion, because while the line number
> > > > implied "crtc->state" being NULL, the drm data structure documentation
> > > > and other drivers both imply that "crtc" was the more likely one.
> > > >
> > > > I suspect a simple
> > > >
> > > >         if (!crtc)
> > > >                 return;
> > > >
> > > > in vc4_hdmi_set_n_cts() is at least part of the fix for this all, but
> > > > I didn't check if there is possibly something else that needs to be
> > > > done too.
> > >
> > > Thanks for the decode_stacktrace.sh and the follow-up
> > >
> > > Yeah, it looks like we have several things wrong here:
> > >
> > >   * we only check that connector->state is set, and not
> > >     connector->state->crtc indeed.
> > >
> > >   * We also check only in startup(), so at open() and not later on when
> > >     the sound streaming actually start. This has been there for a while,
> > >     so I guess it's never really been causing a practical issue before.
> > 
> > You also have no locking
> 
> Indeed. Do we just need locking to prevent a concurrent audio setup and
> modeset, or do you have another corner case in mind?
> 
> Also, generally, what locks should we make sure we have locked when
> accessing the connector and CRTC state? drm_mode_config.connection_mutex
> and drm_mode_config.mutex, respectively?
> 
> > plus looking at ->state objects outside of atomic commit machinery
> > makes no sense because you're not actually in sync with the hw state.
> > Relevant bits need to be copied over at commit time, protected by some
> > spinlock (and that spinlock also needs to be held over whatever other
> > stuff you're setting to make sure we don't get a funny out-of-sync
> > state anywhere).
> 
> If we already have a lock protecting against having both an ASoC and KMS
> function running, it's not clear to me what the spinlock would prevent
> here?

Replicating the irc chat here. With

commit 6c5ed5ae353cdf156f9ac4db17e15db56b4de880
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Thu Apr 6 20:55:20 2017 +0200

    drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.

this is already taken care of for drivers and should be all good from a
locking pov.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch