All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Maxime Ripard <maxime@cerno.tech>
Cc: dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>,
	David Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Rob Clark <robdclark@gmail.com>,
	linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org
Subject: Re: [PATCH v3 10/11] drm: Use state helper instead of the plane state pointer
Date: Mon, 29 Mar 2021 18:52:01 -0700	[thread overview]
Message-ID: <161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20210219120032.260676-10-maxime@cerno.tech>

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()?

> 
> Use the drm_atomic_get_new_plane_state helper to get that state to make it
> more obvious.
> 
> This was made using the coccinelle script below:
> 
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> (
>  static const struct drm_plane_helper_funcs helpers = {
>         ...,
>         .atomic_disable = func,
>         ...,
>  };
> |
>  static const struct drm_plane_helper_funcs helpers = {
>         ...,
>         .atomic_update = func,
>         ...,
>  };
> )
> 
> @ adds_new_state @
> identifier plane_atomic_func.func;
> identifier plane, state;
> identifier new_state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_atomic_state *state)
>  {
>         ...
> -       struct drm_plane_state *new_state = plane->state;
> +       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
>         ...
>  }
> 
> @ include depends on adds_new_state @
> @@
> 
>  #include <drm/drm_atomic.h>
> 
> @ no_include depends on !include && adds_new_state @
> @@
> 
> + #include <drm/drm_atomic.h>
>   #include <drm/...>
> 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c       | 3 ++-
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c      | 4 +++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c      | 3 ++-
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 31071f9e21d7..e8ce72fe54a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1244,7 +1244,8 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
>                                 struct drm_atomic_state *state)
>  {
>         struct dpu_plane *pdpu = to_dpu_plane(plane);
> -       struct drm_plane_state *new_state = plane->state;
> +       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
> +                                                                          plane);
>  
>         pdpu->is_error = false;
>  

This is oopsing for me. It turns out that 'new_state' is NULL. According
to the comments drm_atomic_get_new_plane_state() can return NULL if the
plane isn't part of the global state. I haven't looked much further but
wanted to report it here in case that type of return value makes sense.

If I revert this patch from linux-next my display works and doesn't
crash the system. Or I can check for NULL in the if below and it also
works.

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index df7f3d3afd8b..f31b89531f6a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1251,7 +1251,7 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
 
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
-	if (!new_state->visible) {
+	if (new_state && !new_state->visible) {
 		_dpu_plane_atomic_disable(plane);
 	} else {
 		dpu_plane_sspp_atomic_update(plane);

WARNING: multiple messages have this Message-ID
From: Stephen Boyd <swboyd@chromium.org>
To: Maxime Ripard <maxime@cerno.tech>
Cc: David Airlie <airlied@linux.ie>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.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: Mon, 29 Mar 2021 18:52:01 -0700	[thread overview]
Message-ID: <161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20210219120032.260676-10-maxime@cerno.tech>

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()?

> 
> Use the drm_atomic_get_new_plane_state helper to get that state to make it
> more obvious.
> 
> This was made using the coccinelle script below:
> 
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> (
>  static const struct drm_plane_helper_funcs helpers = {
>         ...,
>         .atomic_disable = func,
>         ...,
>  };
> |
>  static const struct drm_plane_helper_funcs helpers = {
>         ...,
>         .atomic_update = func,
>         ...,
>  };
> )
> 
> @ adds_new_state @
> identifier plane_atomic_func.func;
> identifier plane, state;
> identifier new_state;
> @@
> 
>  func(struct drm_plane *plane, struct drm_atomic_state *state)
>  {
>         ...
> -       struct drm_plane_state *new_state = plane->state;
> +       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
>         ...
>  }
> 
> @ include depends on adds_new_state @
> @@
> 
>  #include <drm/drm_atomic.h>
> 
> @ no_include depends on !include && adds_new_state @
> @@
> 
> + #include <drm/drm_atomic.h>
>   #include <drm/...>
> 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c       | 3 ++-
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c      | 4 +++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c      | 3 ++-
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 31071f9e21d7..e8ce72fe54a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1244,7 +1244,8 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
>                                 struct drm_atomic_state *state)
>  {
>         struct dpu_plane *pdpu = to_dpu_plane(plane);
> -       struct drm_plane_state *new_state = plane->state;
> +       struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
> +                                                                          plane);
>  
>         pdpu->is_error = false;
>  

This is oopsing for me. It turns out that 'new_state' is NULL. According
to the comments drm_atomic_get_new_plane_state() can return NULL if the
plane isn't part of the global state. I haven't looked much further but
wanted to report it here in case that type of return value makes sense.

If I revert this patch from linux-next my display works and doesn't
crash the system. Or I can check for NULL in the if below and it also
works.

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index df7f3d3afd8b..f31b89531f6a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1251,7 +1251,7 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
 
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
-	if (!new_state->visible) {
+	if (new_state && !new_state->visible) {
 		_dpu_plane_atomic_disable(plane);
 	} else {
 		dpu_plane_sspp_atomic_update(plane);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-03-30  1:52 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 [this message]
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
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=161706912161.3012082.17313817257247946143@swboyd.mtv.corp.google.com \
    --to=swboyd@chromium.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=maarten.lankhorst@linux.intel.com \
    --cc=maxime@cerno.tech \
    --cc=robdclark@gmail.com \
    --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.