diff mbox series

[2/2] drm/i915/dp: Modeset only the tiled connectors with CRTC

Message ID 20200123002415.31478-2-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/dp: Do not set master_trans bit in bitmak if INVALID_TRANSCODER | expand

Commit Message

Navare, Manasi Jan. 23, 2020, 12:24 a.m. UTC 
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(-)

Comments

Ville Syrjälä Jan. 23, 2020, 6:27 p.m. UTC | #1 
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
Navare, Manasi Jan. 23, 2020, 10:47 p.m. UTC | #2 
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
Navare, Manasi Jan. 24, 2020, 9:22 p.m. UTC | #3 
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
diff mbox series

Patch

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)