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

[Manasi Navare <manasi.d.navare@intel.com>]

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