All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Daniel Vetter <daniel@ffwll.ch>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
	Emma Anholt <emma@anholt.net>, David Airlie <airlied@linux.ie>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: Regression with mainline kernel on rpi4
Date: Thu, 30 Sep 2021 11:19:59 +0200	[thread overview]
Message-ID: <YVWBP5ZJInH/wt1P@phenom.ffwll.local> (raw)
In-Reply-To: <20210928083446.cfji7hmndt6a5nop@gilmour>

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

  reply	other threads:[~2021-09-30  9:20 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-02  9:08 Sudip Mukherjee
2021-09-02  9:08 ` Sudip Mukherjee
2021-09-03 16:03 ` Maxime Ripard
2021-09-03 20:09   ` Sudip Mukherjee
2021-09-03 20:09     ` Sudip Mukherjee
2021-09-04  9:10     ` Maxime Ripard
2021-09-04  9:40       ` Sudip Mukherjee
2021-09-04  9:40         ` Sudip Mukherjee
2021-09-20 14:47         ` Maxime Ripard
2021-09-20 15:43           ` Maxime Ripard
2021-09-20 15:53             ` Maxime Ripard
2021-09-20 16:35               ` Sudip Mukherjee
2021-09-20 16:35                 ` Sudip Mukherjee
2021-09-20 17:10                 ` Maxime Ripard
2021-09-20 17:21                   ` Sudip Mukherjee
2021-09-20 17:21                     ` Sudip Mukherjee
2021-09-22  9:57                     ` Maxime Ripard
2021-09-22 10:10                       ` Sudip Mukherjee
2021-09-22 10:10                         ` Sudip Mukherjee
2021-09-22 11:28                         ` Maxime Ripard
2021-09-22 17:18                           ` Sudip Mukherjee
2021-09-22 17:18                             ` Sudip Mukherjee
2021-09-22 15:22                         ` Linus Torvalds
2021-09-22 15:22                           ` Linus Torvalds
2021-09-22 17:02                           ` Sudip Mukherjee
2021-09-22 17:02                             ` Sudip Mukherjee
2021-09-22 18:23                             ` Linus Torvalds
2021-09-22 18:23                               ` Linus Torvalds
2021-09-22 20:19                               ` Sudip Mukherjee
2021-09-22 20:19                                 ` Sudip Mukherjee
2021-09-22 20:25                                 ` Linus Torvalds
2021-09-22 20:25                                   ` Linus Torvalds
2021-09-24 13:30                                   ` Maxime Ripard
2021-09-24 22:50                                     ` Daniel Vetter
2021-09-24 22:50                                       ` Daniel Vetter
2021-09-28  8:34                                       ` Maxime Ripard
2021-09-30  9:19                                         ` Daniel Vetter [this message]
2021-10-13 15:01                                           ` Maxime Ripard
2021-10-14 13:15                                             ` Daniel Vetter
2021-10-25 14:15                                               ` Maxime Ripard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVWBP5ZJInH/wt1P@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=emma@anholt.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maxime@cerno.tech \
    --cc=p.zabel@pengutronix.de \
    --cc=sudipm.mukherjee@gmail.com \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: Regression with mainline kernel on rpi4' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.