[Intel-gfx] [PATCH] drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

[Intel-gfx] [PATCH] drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

[Stanislav Lisovskiy]

There is a suspicion that we might not have all bigjoiner pipes
set in correspodent mask, which leads to that not all crtc are added to the state,
however because we are copying for instance crtc reference from master crtc
to slave crtc, we might be trying to get it via intel_atomic_get_new_crtc_state,
which might the return NULL.
This is surely not a fix, but at least the WARN should give us some clue and
"red light" when this happens.
In future we might need to evaluate the logic of adding crtc to the state,
to make sure that we always have all affected crtcs in the state,
even though such functions already exist, there seem to be still some
glitches in this logic.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_atomic_plane.c | 13 +++++++++++++
 drivers/gpu/drm/i915/display/intel_display.c      |  2 +-
 drivers/gpu/drm/i915/display/intel_display.h      |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 4125ee07a271..03cbd755261b 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -695,6 +695,19 @@ int intel_plane_atomic_check(struct intel_atomic_state *state,
 
 		new_master_plane_state =
 			intel_atomic_get_new_plane_state(state, master_plane);
+
+		/*
+		 * We would be copying plane state from master crtc
+		 * however if crtc_state->bigjoiner_pipes doesn't contain both
+		 * master and slave, that means that quite likely we didn't call
+		 * intel_atomic_get_crtc_state for both, which can cause issues,
+		 * like intel_atomic_get_new_crtc_state returning NULL suddently,
+		 * when we for example try to use hw.crtc from that plane state.
+		 * This WARN might sched some light on out existing issues, also
+		 * prevent others from happening in future.
+		 */
+		drm_WARN_ON(state->base.dev, intel_bigjoiner_num_pipes(new_crtc_state) < 2);
+
 	} else {
 		new_master_plane_state = new_plane_state;
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1d5d42a40803..0e0908c1ee8d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -267,7 +267,7 @@ bool intel_crtc_is_bigjoiner_master(const struct intel_crtc_state *crtc_state)
 		crtc->pipe == bigjoiner_master_pipe(crtc_state);
 }
 
-static int intel_bigjoiner_num_pipes(const struct intel_crtc_state *crtc_state)
+int intel_bigjoiner_num_pipes(const struct intel_crtc_state *crtc_state)
 {
 	return hweight8(crtc_state->bigjoiner_pipes);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index ac95961f68ba..94761be69bd0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -410,6 +410,7 @@ bool is_trans_port_sync_mode(const struct intel_crtc_state *state);
 bool intel_crtc_is_bigjoiner_slave(const struct intel_crtc_state *crtc_state);
 bool intel_crtc_is_bigjoiner_master(const struct intel_crtc_state *crtc_state);
 u8 intel_crtc_bigjoiner_slave_pipes(const struct intel_crtc_state *crtc_state);
+int intel_bigjoiner_num_pipes(const struct intel_crtc_state *crtc_state);
 struct intel_crtc *intel_master_crtc(const struct intel_crtc_state *crtc_state);
 bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
 bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

[Patchwork]

== Series Details ==

Series: drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state
URL   : https://patchwork.freedesktop.org/series/117512/
State : warning

== Summary ==

Error: dim checkpatch failed
9de21382c485 drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state
-:8: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#8: 
set in correspodent mask, which leads to that not all crtc are added to the state,

total: 0 errors, 1 warnings, 0 checks, 34 lines checked

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5884 bytes --]

== Series Details ==

Series: drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state
URL   : https://patchwork.freedesktop.org/series/117512/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13125 -> Patchwork_117512v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_117512v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_117512v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/index.html

Participating hosts (40 -> 39)
------------------------------

  Missing    (1): fi-kbl-soraka 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_117512v1:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_psr@primary_mmap_gtt:
    - bat-rplp-1:         NOTRUN -> [ABORT][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-rplp-1/igt@kms_psr@primary_mmap_gtt.html

  
Known issues
------------

  Here are the changes found in Patchwork_117512v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@requests:
    - bat-rpls-2:         [PASS][2] -> [ABORT][3] ([i915#4983] / [i915#7913])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13125/bat-rpls-2/igt@i915_selftest@live@requests.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-rpls-2/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         [PASS][4] -> [DMESG-WARN][5] ([i915#6367] / [i915#7953])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13125/bat-rpls-1/igt@i915_selftest@live@slpc.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-rpls-1/igt@i915_selftest@live@slpc.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-atsm-1:         NOTRUN -> [SKIP][6] ([i915#6645])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-atsm-1/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-atsm-1:         NOTRUN -> [SKIP][7] ([i915#6078])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-atsm-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][8] -> [FAIL][9] ([i915#7932])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13125/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-atsm-1:         NOTRUN -> [SKIP][10] ([i915#1836])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-atsm-1/igt@kms_pipe_crc_basic@suspend-read-crc.html

  
#### Possible fixes ####

  * igt@gem_lmem_swapping@parallel-random-engines@lmem0:
    - bat-atsm-1:         [ABORT][11] -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13125/bat-atsm-1/igt@gem_lmem_swapping@parallel-random-engines@lmem0.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-atsm-1/igt@gem_lmem_swapping@parallel-random-engines@lmem0.html

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [DMESG-WARN][13] ([i915#7699] / [i915#7953]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13125/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-dg2-11/igt@i915_selftest@live@migrate.html

  
#### Warnings ####

  * igt@kms_psr@sprite_plane_onoff:
    - bat-rplp-1:         [ABORT][15] ([i915#8198]) -> [SKIP][16] ([i915#1072])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13125/bat-rplp-1/igt@kms_psr@sprite_plane_onoff.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/bat-rplp-1/igt@kms_psr@sprite_plane_onoff.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7953]: https://gitlab.freedesktop.org/drm/intel/issues/7953
  [i915#8198]: https://gitlab.freedesktop.org/drm/intel/issues/8198


Build changes
-------------

  * Linux: CI_DRM_13125 -> Patchwork_117512v1

  CI-20190529: 20190529
  CI_DRM_13125: 0e421da2a8ce8d16030822ab99bd2a88529a3c8a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7283: ce51f53938690f581b315fa045d41155a5c6ecd3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_117512v1: 0e421da2a8ce8d16030822ab99bd2a88529a3c8a @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

05e257916a91 drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117512v1/index.html

[-- Attachment #2: Type: text/html, Size: 6747 bytes --]

Re: [Intel-gfx] [PATCH] drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

[Ville Syrjälä]

On Tue, May 09, 2023 at 02:14:41PM +0300, Stanislav Lisovskiy wrote:
> There is a suspicion that we might not have all bigjoiner pipes
> set in correspodent mask, which leads to that not all crtc are added to the state,
> however because we are copying for instance crtc reference from master crtc
> to slave crtc, we might be trying to get it via intel_atomic_get_new_crtc_state,
> which might the return NULL.
> This is surely not a fix, but at least the WARN should give us some clue and
> "red light" when this happens.
> In future we might need to evaluate the logic of adding crtc to the state,
> to make sure that we always have all affected crtcs in the state,
> even though such functions already exist, there seem to be still some
> glitches in this logic.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic_plane.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c      |  2 +-
>  drivers/gpu/drm/i915/display/intel_display.h      |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 4125ee07a271..03cbd755261b 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -695,6 +695,19 @@ int intel_plane_atomic_check(struct intel_atomic_state *state,
>  
>  		new_master_plane_state =
>  			intel_atomic_get_new_plane_state(state, master_plane);
> +
> +		/*
> +		 * We would be copying plane state from master crtc
> +		 * however if crtc_state->bigjoiner_pipes doesn't contain both
> +		 * master and slave, that means that quite likely we didn't call
> +		 * intel_atomic_get_crtc_state for both, which can cause issues,
> +		 * like intel_atomic_get_new_crtc_state returning NULL suddently,
> +		 * when we for example try to use hw.crtc from that plane state.
> +		 * This WARN might sched some light on out existing issues, also
> +		 * prevent others from happening in future.
> +		 */
> +		drm_WARN_ON(state->base.dev, intel_bigjoiner_num_pipes(new_crtc_state) < 2);

What you are doing here is basically just
 if (bigjoiner_pipes)
  	assert(bigjoiner_pipes != 1);
which is not going to catch anything.

We can trivially see that it will never happen given
how bigjoiner_pipes is initialized.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm/i915: WARN if not all pipes are in bigjoiner mask, when copying plane state

[Lisovskiy, Stanislav]

On Wed, May 10, 2023 at 04:47:57PM +0300, Ville Syrjälä wrote:
> On Tue, May 09, 2023 at 02:14:41PM +0300, Stanislav Lisovskiy wrote:
> > There is a suspicion that we might not have all bigjoiner pipes
> > set in correspodent mask, which leads to that not all crtc are added to the state,
> > however because we are copying for instance crtc reference from master crtc
> > to slave crtc, we might be trying to get it via intel_atomic_get_new_crtc_state,
> > which might the return NULL.
> > This is surely not a fix, but at least the WARN should give us some clue and
> > "red light" when this happens.
> > In future we might need to evaluate the logic of adding crtc to the state,
> > to make sure that we always have all affected crtcs in the state,
> > even though such functions already exist, there seem to be still some
> > glitches in this logic.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_atomic_plane.c | 13 +++++++++++++
> >  drivers/gpu/drm/i915/display/intel_display.c      |  2 +-
> >  drivers/gpu/drm/i915/display/intel_display.h      |  1 +
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 4125ee07a271..03cbd755261b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -695,6 +695,19 @@ int intel_plane_atomic_check(struct intel_atomic_state *state,
> >  
> >  		new_master_plane_state =
> >  			intel_atomic_get_new_plane_state(state, master_plane);
> > +
> > +		/*
> > +		 * We would be copying plane state from master crtc
> > +		 * however if crtc_state->bigjoiner_pipes doesn't contain both
> > +		 * master and slave, that means that quite likely we didn't call
> > +		 * intel_atomic_get_crtc_state for both, which can cause issues,
> > +		 * like intel_atomic_get_new_crtc_state returning NULL suddently,
> > +		 * when we for example try to use hw.crtc from that plane state.
> > +		 * This WARN might sched some light on out existing issues, also
> > +		 * prevent others from happening in future.
> > +		 */
> > +		drm_WARN_ON(state->base.dev, intel_bigjoiner_num_pipes(new_crtc_state) < 2);
> 
> What you are doing here is basically just
>  if (bigjoiner_pipes)
>   	assert(bigjoiner_pipes != 1);
> which is not going to catch anything.
> 
> We can trivially see that it will never happen given
> how bigjoiner_pipes is initialized.

intel_bigjoiner_num_pipes counts how many pipe are used, 
as I understand in case of bigjoiner we should always have it 
returning 2, right?

For example intel_bigjoiner_adjust_timings uses it currently
same way in our code, also I think there other places:

static void intel_bigjoiner_adjust_timings(const struct intel_crtc_state *crtc_state,
					   struct drm_display_mode *mode)
{
	int num_pipes = intel_bigjoiner_num_pipes(crtc_state);

	if (num_pipes < 2)
		return;

...

I was just willing to check if, we might have this mask
messed up somehow, because we add crtcs to the state based
on this mask. 

However I already checked, problem seems to be
somewhere else. 

Stan

> 
> -- 
> Ville Syrjälä
> Intel