[Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Manasi Navare]

In the port sync mode, for the master crtc, the master_transcoder is INVALID.
In that case since its value is -1, do not set the bit in the bitmask.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 878d331b9e8c..79f9054078ea 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
 		}
 
 		if (is_trans_port_sync_mode(new_crtc_state)) {
-			u8 trans = new_crtc_state->sync_mode_slaves_mask |
-				   BIT(new_crtc_state->master_transcoder);
+			u8 trans = new_crtc_state->sync_mode_slaves_mask;
+
+			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
+				trans |= BIT(new_crtc_state->master_transcoder);
 
 			if (intel_cpu_transcoders_need_modeset(state, trans)) {
 				new_crtc_state->uapi.mode_changed = true;
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915/dp: Modeset only the tiled connectors with CRTC

[Manasi Navare]

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Fixes: a603f5bd1691 ("drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset")
Closes: https://gitlab.freedesktop.org/drm/intel/issues/516
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 79f9054078ea..c8d6f6e8fc7f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13155,11 +13155,12 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
 	}
 
 	/* Get total number of tiled connectors in state that belong to
-	 * this tile group.
+	 * this tile group and that have a CRTC
 	 */
 	for_each_new_connector_in_state(state, connector, connector_state, i) {
 		if (connector->has_tile &&
-		    connector->tile_group->id == tile_group_id)
+		    connector->tile_group->id == tile_group_id &&
+		    connector_state->crtc)
 			num_tiled_conns++;
 	}
 
@@ -14507,13 +14508,14 @@ intel_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
 			continue;
 		conn_state = drm_atomic_get_connector_state(&state->base,
 							    connector);
+
 		if (IS_ERR(conn_state)) {
 			ret =  PTR_ERR(conn_state);
 			break;
 		}
 
 		if (!conn_state->crtc)
-			continue;
+			break;
 
 		crtc_state = drm_atomic_get_crtc_state(&state->base,
 						       conn_state->crtc);
@@ -14550,6 +14552,8 @@ intel_atomic_check_tiled_conns(struct intel_atomic_state *state)
 			continue;
 		if (!intel_connector_needs_modeset(state, connector))
 			continue;
+		if (!new_conn_state->crtc)
+			continue;
 
 		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
 		if (ret)
-- 
2.19.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Modeset only the tiled connectors with CRTC

[Ville Syrjälä]

On Wed, Jan 22, 2020 at 04:24:15PM -0800, Manasi Navare wrote:

Missing commit msg.


> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: a603f5bd1691 ("drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset")
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/516
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 79f9054078ea..c8d6f6e8fc7f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13155,11 +13155,12 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
>  	}
>  
>  	/* Get total number of tiled connectors in state that belong to
> -	 * this tile group.
> +	 * this tile group and that have a CRTC
>  	 */
>  	for_each_new_connector_in_state(state, connector, connector_state, i) {
>  		if (connector->has_tile &&
> -		    connector->tile_group->id == tile_group_id)
> +		    connector->tile_group->id == tile_group_id &&
> +		    connector_state->crtc)

It might have a crtc, but that crtc might not be active. So feels like
this fixes things more by luck that by design.

The real problem of course is that we can't tell whether other crtcs are
active or not during intel_modeset_pipe_config(). All pipes must finish
that stage before we can make an actual decision based on what's active
and what's inactive. And that requires the two stage approach I proposed
in my branch before xmas.

>  			num_tiled_conns++;
>  	}
>  
> @@ -14507,13 +14508,14 @@ intel_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
>  			continue;
>  		conn_state = drm_atomic_get_connector_state(&state->base,
>  							    connector);
> +
>  		if (IS_ERR(conn_state)) {
>  			ret =  PTR_ERR(conn_state);
>  			break;
>  		}
>  
>  		if (!conn_state->crtc)
> -			continue;
> +			break;
>  
>  		crtc_state = drm_atomic_get_crtc_state(&state->base,
>  						       conn_state->crtc);
> @@ -14550,6 +14552,8 @@ intel_atomic_check_tiled_conns(struct intel_atomic_state *state)
>  			continue;
>  		if (!intel_connector_needs_modeset(state, connector))
>  			continue;
> +		if (!new_conn_state->crtc)
> +			continue;
>  
>  		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
>  		if (ret)
> -- 
> 2.19.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Modeset only the tiled connectors with CRTC

[Manasi Navare]

On Thu, Jan 23, 2020 at 08:27:12PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 22, 2020 at 04:24:15PM -0800, Manasi Navare wrote:
> 
> Missing commit msg.

Yes will add the commit message

> 
> 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Fixes: a603f5bd1691 ("drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset")
> > Closes: https://gitlab.freedesktop.org/drm/intel/issues/516
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 79f9054078ea..c8d6f6e8fc7f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13155,11 +13155,12 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> >  	}
> >  
> >  	/* Get total number of tiled connectors in state that belong to
> > -	 * this tile group.
> > +	 * this tile group and that have a CRTC
> >  	 */
> >  	for_each_new_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector->has_tile &&
> > -		    connector->tile_group->id == tile_group_id)
> > +		    connector->tile_group->id == tile_group_id &&
> > +		    connector_state->crtc)
> 
> It might have a crtc, but that crtc might not be active. So feels like
> this fixes things more by luck that by design.

In case of kms_flip test cases, the second connector did not have crtc and it was
still getting added to the state trying to do commit on that causing it to fail.

So adding this check fixes it. So I think this fix is definitely needed plus I will look at
adding a check for crtc_active using your suggested approach in your branch too.

> 
> The real problem of course is that we can't tell whether other crtcs are
> active or not during intel_modeset_pipe_config(). All pipes must finish
> that stage before we can make an actual decision based on what's active
> and what's inactive. And that requires the two stage approach I proposed
> in my branch before xmas.

Perhaps I was thinking of adding another check after intel_modeset_pipe_config() but in intel_atomic_check()
something like intel_port_sync_check() where we count the number of conns with active crtc and if that is
less than the num tiles and if trans_port_sync mode, then reset port sync mode assignments so it commits
in the normal non port sync mode.
What do you think of this approach? I will also take a look at your branch and try to understand that.

Manasi

> 
> >  			num_tiled_conns++;
> >  	}
> >  
> > @@ -14507,13 +14508,14 @@ intel_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> >  			continue;
> >  		conn_state = drm_atomic_get_connector_state(&state->base,
> >  							    connector);
> > +
> >  		if (IS_ERR(conn_state)) {
> >  			ret =  PTR_ERR(conn_state);
> >  			break;
> >  		}
> >  
> >  		if (!conn_state->crtc)
> > -			continue;
> > +			break;
> >  
> >  		crtc_state = drm_atomic_get_crtc_state(&state->base,
> >  						       conn_state->crtc);
> > @@ -14550,6 +14552,8 @@ intel_atomic_check_tiled_conns(struct intel_atomic_state *state)
> >  			continue;
> >  		if (!intel_connector_needs_modeset(state, connector))
> >  			continue;
> > +		if (!new_conn_state->crtc)
> > +			continue;
> >  
> >  		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> >  		if (ret)
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: Modeset only the tiled connectors with CRTC

[Manasi Navare]

On Thu, Jan 23, 2020 at 02:47:04PM -0800, Manasi Navare wrote:
> On Thu, Jan 23, 2020 at 08:27:12PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 22, 2020 at 04:24:15PM -0800, Manasi Navare wrote:
> > 
> > Missing commit msg.
> 
> Yes will add the commit message
>

Capturing an Ack from Ville from IRC after adding the commit message:

Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Manasi
 
> > 
> > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Fixes: a603f5bd1691 ("drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset")
> > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/516
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 79f9054078ea..c8d6f6e8fc7f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13155,11 +13155,12 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > >  	}
> > >  
> > >  	/* Get total number of tiled connectors in state that belong to
> > > -	 * this tile group.
> > > +	 * this tile group and that have a CRTC
> > >  	 */
> > >  	for_each_new_connector_in_state(state, connector, connector_state, i) {
> > >  		if (connector->has_tile &&
> > > -		    connector->tile_group->id == tile_group_id)
> > > +		    connector->tile_group->id == tile_group_id &&
> > > +		    connector_state->crtc)
> > 
> > It might have a crtc, but that crtc might not be active. So feels like
> > this fixes things more by luck that by design.
> 
> In case of kms_flip test cases, the second connector did not have crtc and it was
> still getting added to the state trying to do commit on that causing it to fail.
> 
> So adding this check fixes it. So I think this fix is definitely needed plus I will look at
> adding a check for crtc_active using your suggested approach in your branch too.
> 
> > 
> > The real problem of course is that we can't tell whether other crtcs are
> > active or not during intel_modeset_pipe_config(). All pipes must finish
> > that stage before we can make an actual decision based on what's active
> > and what's inactive. And that requires the two stage approach I proposed
> > in my branch before xmas.
> 
> Perhaps I was thinking of adding another check after intel_modeset_pipe_config() but in intel_atomic_check()
> something like intel_port_sync_check() where we count the number of conns with active crtc and if that is
> less than the num tiles and if trans_port_sync mode, then reset port sync mode assignments so it commits
> in the normal non port sync mode.
> What do you think of this approach? I will also take a look at your branch and try to understand that.
> 
> Manasi
> 
> > 
> > >  			num_tiled_conns++;
> > >  	}
> > >  
> > > @@ -14507,13 +14508,14 @@ intel_modeset_all_tiles(struct intel_atomic_state *state, int tile_grp_id)
> > >  			continue;
> > >  		conn_state = drm_atomic_get_connector_state(&state->base,
> > >  							    connector);
> > > +
> > >  		if (IS_ERR(conn_state)) {
> > >  			ret =  PTR_ERR(conn_state);
> > >  			break;
> > >  		}
> > >  
> > >  		if (!conn_state->crtc)
> > > -			continue;
> > > +			break;
> > >  
> > >  		crtc_state = drm_atomic_get_crtc_state(&state->base,
> > >  						       conn_state->crtc);
> > > @@ -14550,6 +14552,8 @@ intel_atomic_check_tiled_conns(struct intel_atomic_state *state)
> > >  			continue;
> > >  		if (!intel_connector_needs_modeset(state, connector))
> > >  			continue;
> > > +		if (!new_conn_state->crtc)
> > > +			continue;
> > >  
> > >  		ret = intel_modeset_all_tiles(state, connector->tile_group->id);
> > >  		if (ret)
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER
URL   : https://patchwork.freedesktop.org/series/72435/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e79c35cfb8ba drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER
-:10: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#10: 
In the port sync mode, for the master crtc, the master_transcoder is INVALID.

total: 0 errors, 1 warnings, 0 checks, 12 lines checked
6f283f131d51 drm/i915/dp: Modeset only the tiled connectors with CRTC

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER
URL   : https://patchwork.freedesktop.org/series/72435/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7799 -> Patchwork_16224
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16224 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16224, 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_16224/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_parallel@contexts:
    - fi-byt-j1900:       NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-byt-j1900/igt@gem_exec_parallel@contexts.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_parallel@fds:
    - fi-byt-n2820:       [PASS][2] -> [TIMEOUT][3] ([fdo#112271])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-byt-n2820/igt@gem_exec_parallel@fds.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-byt-n2820/igt@gem_exec_parallel@fds.html

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-skl-6700k2:      [PASS][4] -> [INCOMPLETE][5] ([i915#671])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-skl-6700k2/igt@i915_module_load@reload-with-fault-injection.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-skl-6700k2/igt@i915_module_load@reload-with-fault-injection.html
    - fi-skl-lmem:        [PASS][6] -> [INCOMPLETE][7] ([i915#671])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-skl-lmem/igt@i915_module_load@reload-with-fault-injection.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-skl-lmem/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][8] -> [FAIL][9] ([fdo#111407])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-j1900:       [INCOMPLETE][10] ([i915#45]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-byt-j1900/igt@gem_close_race@basic-threads.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-byt-j1900/igt@gem_close_race@basic-threads.html

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-cfl-guc:         [INCOMPLETE][12] ([i915#505] / [i915#671]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-cfl-guc/igt@i915_module_load@reload-with-fault-injection.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-cfl-guc/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770r:       [DMESG-FAIL][14] ([i915#553] / [i915#725]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7799/fi-hsw-4770r/igt@i915_selftest@live_blt.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/fi-hsw-4770r/igt@i915_selftest@live_blt.html

  
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45
  [i915#505]: https://gitlab.freedesktop.org/drm/intel/issues/505
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#671]: https://gitlab.freedesktop.org/drm/intel/issues/671
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725


Participating hosts (50 -> 42)
------------------------------

  Additional (2): fi-glk-dsi fi-ilk-650 
  Missing    (10): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-kbl-x1275 fi-bdw-samus fi-icl-dsi fi-skl-6600u 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7799 -> Patchwork_16224

  CI-20190529: 20190529
  CI_DRM_7799: 0f8a46a25a7781ef6ede604c9cb50f82cfb5e960 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5377: 1e6cb3e75925cf623df04f78430ae9299632ec3f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16224: 6f283f131d51ee27fef97019d96833a011247b3f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6f283f131d51 drm/i915/dp: Modeset only the tiled connectors with CRTC
e79c35cfb8ba drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16224/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Ville Syrjälä]

On Wed, Jan 22, 2020 at 04:24:14PM -0800, Manasi Navare wrote:
> In the port sync mode, for the master crtc, the master_transcoder is INVALID.
> In that case since its value is -1, do not set the bit in the bitmask.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 878d331b9e8c..79f9054078ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> -			u8 trans = new_crtc_state->sync_mode_slaves_mask |
> -				   BIT(new_crtc_state->master_transcoder);
> +			u8 trans = new_crtc_state->sync_mode_slaves_mask;
> +
> +			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> +				trans |= BIT(new_crtc_state->master_transcoder);
>  
>  			if (intel_cpu_transcoders_need_modeset(state, trans)) {
>  				new_crtc_state->uapi.mode_changed = true;
> -- 
> 2.19.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Manasi Navare]

On Thu, Jan 23, 2020 at 04:01:28PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 22, 2020 at 04:24:14PM -0800, Manasi Navare wrote:
> > In the port sync mode, for the master crtc, the master_transcoder is INVALID.
> > In that case since its value is -1, do not set the bit in the bitmask.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review, I think populating master transcoder in master would also need us
to change all the helpers for is_trans_port_sync_mode and the readout and the commit_modeset_enables logic.
Its doable but too risky to change at this point incase it causes any regressions.

Still assume your r-b on this?

Manasi
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 878d331b9e8c..79f9054078ea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  		}
> >  
> >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > -			u8 trans = new_crtc_state->sync_mode_slaves_mask |
> > -				   BIT(new_crtc_state->master_transcoder);
> > +			u8 trans = new_crtc_state->sync_mode_slaves_mask;
> > +
> > +			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> > +				trans |= BIT(new_crtc_state->master_transcoder);
> >  
> >  			if (intel_cpu_transcoders_need_modeset(state, trans)) {
> >  				new_crtc_state->uapi.mode_changed = true;
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Souza, Jose]

On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> In the port sync mode, for the master crtc, the master_transcoder is
> INVALID.
> In that case since its value is -1, do not set the bit in the
> bitmask.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> port sync")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 878d331b9e8c..79f9054078ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> drm_device *dev,
>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> -			u8 trans = new_crtc_state-
> >sync_mode_slaves_mask |
> -				   BIT(new_crtc_state-
> >master_transcoder);
> +			u8 trans = new_crtc_state-
> >sync_mode_slaves_mask;
> +
> +			if (new_crtc_state->master_transcoder !=
> INVALID_TRANSCODER)
> +				trans |= BIT(new_crtc_state-
> >master_transcoder);

Why not set master_transcoder in port sync master too? Would avoid have
this check here and in future other places.

>  
>  			if (intel_cpu_transcoders_need_modeset(state,
> trans)) {
>  				new_crtc_state->uapi.mode_changed =
> true;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Ville Syrjälä]

On Thu, Jan 23, 2020 at 06:25:29PM +0000, Souza, Jose wrote:
> On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> > In the port sync mode, for the master crtc, the master_transcoder is
> > INVALID.
> > In that case since its value is -1, do not set the bit in the
> > bitmask.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> > port sync")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 878d331b9e8c..79f9054078ea 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  		}
> >  
> >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > -			u8 trans = new_crtc_state-
> > >sync_mode_slaves_mask |
> > -				   BIT(new_crtc_state-
> > >master_transcoder);
> > +			u8 trans = new_crtc_state-
> > >sync_mode_slaves_mask;
> > +
> > +			if (new_crtc_state->master_transcoder !=
> > INVALID_TRANSCODER)
> > +				trans |= BIT(new_crtc_state-
> > >master_transcoder);
> 
> Why not set master_transcoder in port sync master too? Would avoid have
> this check here and in future other places.

Not how the hardware works. So would complicate hw readout and
programming code needlessly.

> 
> >  
> >  			if (intel_cpu_transcoders_need_modeset(state,
> > trans)) {
> >  				new_crtc_state->uapi.mode_changed =
> > true;

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Ville Syrjälä]

On Thu, Jan 23, 2020 at 08:31:55PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 23, 2020 at 06:25:29PM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> > > In the port sync mode, for the master crtc, the master_transcoder is
> > > INVALID.
> > > In that case since its value is -1, do not set the bit in the
> > > bitmask.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> > > port sync")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 878d331b9e8c..79f9054078ea 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  		}
> > >  
> > >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > -			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask |
> > > -				   BIT(new_crtc_state-
> > > >master_transcoder);
> > > +			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask;
> > > +
> > > +			if (new_crtc_state->master_transcoder !=
> > > INVALID_TRANSCODER)
> > > +				trans |= BIT(new_crtc_state-
> > > >master_transcoder);
> > 
> > Why not set master_transcoder in port sync master too? Would avoid have
> > this check here and in future other places.
> 
> Not how the hardware works. So would complicate hw readout and
> programming code needlessly.

Hmm. Actually not that much maybe. Readout already has to trawl
everything to populate the slave bitmask. So the change to programming
part would just be something like

+ if (cpu_transcoder != master_transcoder)
	enable port sync mode;

So could maybe be done. Might make state dumps a bit more confusing
though since we definitely do not enable port sync mode on the
master transcoder.

> 
> > 
> > >  
> > >  			if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > >  				new_crtc_state->uapi.mode_changed =
> > > true;
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Manasi Navare]

On Thu, Jan 23, 2020 at 08:31:55PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 23, 2020 at 06:25:29PM +0000, Souza, Jose wrote:
> > On Wed, 2020-01-22 at 16:24 -0800, Manasi Navare wrote:
> > > In the port sync mode, for the master crtc, the master_transcoder is
> > > INVALID.
> > > In that case since its value is -1, do not set the bit in the
> > > bitmask.
> > > 
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for
> > > port sync")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 878d331b9e8c..79f9054078ea 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct
> > > drm_device *dev,
> > >  		}
> > >  
> > >  		if (is_trans_port_sync_mode(new_crtc_state)) {
> > > -			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask |
> > > -				   BIT(new_crtc_state-
> > > >master_transcoder);
> > > +			u8 trans = new_crtc_state-
> > > >sync_mode_slaves_mask;
> > > +
> > > +			if (new_crtc_state->master_transcoder !=
> > > INVALID_TRANSCODER)
> > > +				trans |= BIT(new_crtc_state-
> > > >master_transcoder);
> > 
> > Why not set master_transcoder in port sync master too? Would avoid have
> > this check here and in future other places.
> 
> Not how the hardware works. So would complicate hw readout and
> programming code needlessly.

Yes and thats how we identify that it is the master since its master trans is INVALID

Manasi

> 
> > 
> > >  
> > >  			if (intel_cpu_transcoders_need_modeset(state,
> > > trans)) {
> > >  				new_crtc_state->uapi.mode_changed =
> > > true;
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER

[Manasi Navare]

Thanks for the review, pushed to dinq

Manasi

On Wed, Jan 22, 2020 at 04:24:14PM -0800, Manasi Navare wrote:
> In the port sync mode, for the master crtc, the master_transcoder is INVALID.
> In that case since its value is -1, do not set the bit in the bitmask.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0eed1545fe7 ("drm/i915: Fix post-fastset modeset check for port sync")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 878d331b9e8c..79f9054078ea 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14649,8 +14649,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  		}
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> -			u8 trans = new_crtc_state->sync_mode_slaves_mask |
> -				   BIT(new_crtc_state->master_transcoder);
> +			u8 trans = new_crtc_state->sync_mode_slaves_mask;
> +
> +			if (new_crtc_state->master_transcoder != INVALID_TRANSCODER)
> +				trans |= BIT(new_crtc_state->master_transcoder);
>  
>  			if (intel_cpu_transcoders_need_modeset(state, trans)) {
>  				new_crtc_state->uapi.mode_changed = true;
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx