All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: Maxime Ripard <maxime@cerno.tech>,
	David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Stephen Boyd <swboyd@chromium.org>,
	abhinavk@codeaurora.org, Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH v3 10/11] drm: Use state helper instead of the plane state pointer
Date: Tue, 4 May 2021 09:40:24 +0200	[thread overview]
Message-ID: <YJD6aFbn/vwQkq+5@phenom.ffwll.local> (raw)
In-Reply-To: <CAF6AEGt8t78WLt=GQ1PFANtOC2thoYiTj7kCrh4cTr+CVH68eQ@mail.gmail.com>

On Fri, Apr 30, 2021 at 09:44:42AM -0700, Rob Clark wrote:
> On Thu, Apr 8, 2021 at 6:20 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Stephen,
> >
> > On Tue, Mar 30, 2021 at 11:56:15AM -0700, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2021-03-30 08:35:27)
> > > > Hi Stephen,
> > > >
> > > > On Mon, Mar 29, 2021 at 06:52:01PM -0700, Stephen Boyd wrote:
> > > > > Trimming Cc list way down, sorry if that's too much.
> > > > >
> > > > > Quoting Maxime Ripard (2021-02-19 04:00:30)
> > > > > > Many drivers reference the plane->state pointer in order to get the
> > > > > > current plane state in their atomic_update or atomic_disable hooks,
> > > > > > which would be the new plane state in the global atomic state since
> > > > > > _swap_state happened when those hooks are run.
> > > > >
> > > > > Does this mean drm_atomic_helper_swap_state()?
> > > >
> > > > Yep. Previous to that call in drm_atomic_helper_commit, plane->state is
> > > > the state currently programmed in the hardware, so the old state (that's
> > > > the case you have with atomic_check for example)
> > > >
> > > > Once drm_atomic_helper_swap_state has run, plane->state is now the state
> > > > that needs to be programmed into the hardware, so the new state.
> > >
> > > Ok, and I suppose that is called by drm_atomic_helper_commit()?
> >
> > Yep :)
> >
> > > So presumably a modeset is causing this? I get the NULL pointer around
> > > the time we switch from the splash screen to the login screen. I think
> > > there's a modeset during that transition.
> >
> > It's very likely yeah. I really don't get how that pointer could be null
> > though :/
> 
> So I think I see what is going on.. the issue is the CRTC has changed,
> but not the plane, so there is no new-state for the plane.

Yeah you're not allowed to touch an object's hw state in ->atomic_commit
without acquiring it's state in atomic_check. Otherwise the
synchronization across commits that atomic helpers provides goes boom.

> But dpu_crtc_atomic_flush() iterates over all the attached planes,
> calling dpu_plane_restore() which leads into
> dpu_plane_atomic_update().. this is kinda dpu breaking the rules..

You're probably missing a drm_atomic_add_affected_planes() somewhere.
Without looking at the code at least, it might be that if you just blindly
do that you take too many states by default and oversynchronize across
multiple crtc, which isn't great. But better than getting the rules wrong
:-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	abhinavk@codeaurora.org, Stephen Boyd <swboyd@chromium.org>,
	Maxime Ripard <maxime@cerno.tech>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH v3 10/11] drm: Use state helper instead of the plane state pointer
Date: Tue, 4 May 2021 09:40:24 +0200	[thread overview]
Message-ID: <YJD6aFbn/vwQkq+5@phenom.ffwll.local> (raw)
In-Reply-To: <CAF6AEGt8t78WLt=GQ1PFANtOC2thoYiTj7kCrh4cTr+CVH68eQ@mail.gmail.com>

On Fri, Apr 30, 2021 at 09:44:42AM -0700, Rob Clark wrote:
> On Thu, Apr 8, 2021 at 6:20 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Stephen,
> >
> > On Tue, Mar 30, 2021 at 11:56:15AM -0700, Stephen Boyd wrote:
> > > Quoting Maxime Ripard (2021-03-30 08:35:27)
> > > > Hi Stephen,
> > > >
> > > > On Mon, Mar 29, 2021 at 06:52:01PM -0700, Stephen Boyd wrote:
> > > > > Trimming Cc list way down, sorry if that's too much.
> > > > >
> > > > > Quoting Maxime Ripard (2021-02-19 04:00:30)
> > > > > > Many drivers reference the plane->state pointer in order to get the
> > > > > > current plane state in their atomic_update or atomic_disable hooks,
> > > > > > which would be the new plane state in the global atomic state since
> > > > > > _swap_state happened when those hooks are run.
> > > > >
> > > > > Does this mean drm_atomic_helper_swap_state()?
> > > >
> > > > Yep. Previous to that call in drm_atomic_helper_commit, plane->state is
> > > > the state currently programmed in the hardware, so the old state (that's
> > > > the case you have with atomic_check for example)
> > > >
> > > > Once drm_atomic_helper_swap_state has run, plane->state is now the state
> > > > that needs to be programmed into the hardware, so the new state.
> > >
> > > Ok, and I suppose that is called by drm_atomic_helper_commit()?
> >
> > Yep :)
> >
> > > So presumably a modeset is causing this? I get the NULL pointer around
> > > the time we switch from the splash screen to the login screen. I think
> > > there's a modeset during that transition.
> >
> > It's very likely yeah. I really don't get how that pointer could be null
> > though :/
> 
> So I think I see what is going on.. the issue is the CRTC has changed,
> but not the plane, so there is no new-state for the plane.

Yeah you're not allowed to touch an object's hw state in ->atomic_commit
without acquiring it's state in atomic_check. Otherwise the
synchronization across commits that atomic helpers provides goes boom.

> But dpu_crtc_atomic_flush() iterates over all the attached planes,
> calling dpu_plane_restore() which leads into
> dpu_plane_atomic_update().. this is kinda dpu breaking the rules..

You're probably missing a drm_atomic_add_affected_planes() somewhere.
Without looking at the code at least, it might be that if you just blindly
do that you take too many states by default and oversynchronize across
multiple crtc, which isn't great. But better than getting the rules wrong
:-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-05-04  7:40 UTC|newest]

Thread overview: 112+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-19 12:00 [PATCH v3 01/11] drm/atomic: Pass the full state to planes async atomic check and update Maxime Ripard
2021-02-19 12:00 ` Maxime Ripard
2021-02-19 12:00 ` Maxime Ripard
2021-02-19 12:00 ` Maxime Ripard
2021-02-19 12:00 ` Maxime Ripard
2021-02-19 12:00 ` Maxime Ripard
2021-02-19 12:00 ` [PATCH v3 02/11] drm: Rename plane atomic_check state names Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` [Nouveau] " Maxime Ripard
2021-02-19 14:49   ` Thomas Zimmermann
2021-02-19 14:49     ` Thomas Zimmermann
2021-02-19 14:49     ` Thomas Zimmermann
2021-02-19 14:49     ` Thomas Zimmermann
2021-02-19 14:49     ` Thomas Zimmermann
2021-02-19 14:49     ` Thomas Zimmermann
2021-02-19 14:49     ` [Nouveau] " Thomas Zimmermann
2021-02-19 15:12     ` Maxime Ripard
2021-02-19 15:12       ` Maxime Ripard
2021-02-19 15:12       ` Maxime Ripard
2021-02-19 15:12       ` Maxime Ripard
2021-02-19 15:12       ` Maxime Ripard
2021-02-19 15:12       ` [Nouveau] " Maxime Ripard
2021-02-22  9:01       ` Thomas Zimmermann
2021-02-22  9:01         ` Thomas Zimmermann
2021-02-22  9:01         ` Thomas Zimmermann
2021-02-22  9:01         ` Thomas Zimmermann
2021-02-22  9:01         ` Thomas Zimmermann
2021-02-22  9:01         ` Thomas Zimmermann
2021-02-22  9:01         ` [Nouveau] " Thomas Zimmermann
2021-02-19 12:00 ` [PATCH v3 03/11] drm/atmel-hlcdc: Rename custom plane state variable Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00 ` [PATCH v3 04/11] drm/atomic: Pass the full state to planes atomic_check Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` [Nouveau] " Maxime Ripard
2021-02-22  9:00   ` Thomas Zimmermann
2021-02-22  9:00     ` Thomas Zimmermann
2021-02-22  9:00     ` Thomas Zimmermann
2021-02-22  9:00     ` Thomas Zimmermann
2021-02-22  9:00     ` Thomas Zimmermann
2021-02-22  9:00     ` Thomas Zimmermann
2021-02-22  9:00     ` [Nouveau] " Thomas Zimmermann
2021-02-19 12:00 ` [PATCH v3 05/11] drm: Use the state pointer directly in " Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-22  9:05   ` Thomas Zimmermann
2021-02-22  9:05     ` Thomas Zimmermann
2021-02-22  9:05     ` Thomas Zimmermann
2021-02-22  9:05     ` Thomas Zimmermann
2021-02-22  9:05     ` Thomas Zimmermann
2021-02-22  9:05     ` Thomas Zimmermann
2021-02-22  9:05     ` Thomas Zimmermann
2021-02-19 12:00 ` [PATCH v3 06/11] drm: Use state helper instead of plane state pointer in atomic_check Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-22  9:12   ` Thomas Zimmermann
2021-02-22  9:12     ` Thomas Zimmermann
2021-02-22  9:12     ` Thomas Zimmermann
2021-02-23 14:41     ` Maxime Ripard
2021-02-23 14:41       ` Maxime Ripard
2021-02-23 14:41       ` Maxime Ripard
2021-02-19 12:00 ` [PATCH v3 07/11] drm: Store new plane state in a variable for atomic_update and disable Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00 ` [PATCH v3 08/11] drm: Rename plane->state variables in atomic update " Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00 ` [PATCH v3 09/11] drm/atomic: Pass the full state to planes atomic disable and update Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00 ` [PATCH v3 10/11] drm: Use state helper instead of the plane state pointer Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-03-30  1:52   ` Stephen Boyd
2021-03-30  1:52     ` Stephen Boyd
2021-03-30 15:35     ` Maxime Ripard
2021-03-30 15:35       ` Maxime Ripard
2021-03-30 18:56       ` Stephen Boyd
2021-03-30 18:56         ` Stephen Boyd
2021-04-08 13:20         ` Maxime Ripard
2021-04-08 13:20           ` Maxime Ripard
2021-04-30 16:44           ` Rob Clark
2021-04-30 16:44             ` Rob Clark
2021-05-04  7:40             ` Daniel Vetter [this message]
2021-05-04  7:40               ` Daniel Vetter
2021-02-19 12:00 ` [PATCH v3 11/11] drm/todo: Remove the drm_atomic_state todo item Maxime Ripard
2021-02-19 12:00   ` Maxime Ripard
2021-02-24 11:33 ` [PATCH v3 01/11] drm/atomic: Pass the full state to planes async atomic check and update Thomas Zimmermann
2021-02-24 11:33   ` Thomas Zimmermann
2021-02-24 11:33   ` Thomas Zimmermann
2021-02-24 11:33   ` Thomas Zimmermann
2021-02-24 11:33   ` Thomas Zimmermann
2021-02-24 11:33   ` Thomas Zimmermann
2021-02-25  7:08   ` Maxime Ripard
2021-02-25  7:08     ` Maxime Ripard
2021-02-25  7:08     ` Maxime Ripard
2021-02-25  7:08     ` Maxime Ripard
2021-02-25  7:08     ` Maxime Ripard
2021-02-25  7:08     ` 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=YJD6aFbn/vwQkq+5@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=abhinavk@codeaurora.org \
    --cc=airlied@linux.ie \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=maxime@cerno.tech \
    --cc=robdclark@gmail.com \
    --cc=swboyd@chromium.org \
    --cc=tzimmermann@suse.de \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.