Re: [Intel-gfx] [PATCH 1/3] drm/i915/dp: Make sure all tiled connectors get added to the state with full modeset

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

On Mon, Dec 16, 2019 at 04:37:38PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:23PM -0800, Manasi Navare wrote:
> > In case of tiled displays, all the tiles are linke dto each other
> > for transcoder port sync. So in intel_atomic_check() we need to make
> > sure that we add all the tiles to the modeset and if one of the
> > tiles needs a full modeset then mark all other tiles for a full modeset.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 803993a01ca7..7263eaa66cda 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14066,6 +14066,80 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static int
> > +intel_dp_modeset_all_tiles(struct drm_i915_private *dev_priv,
> > +			   struct intel_atomic_state *state, int tile_grp_id)
> > +{
> > +	struct drm_connector *conn_iter;
> > +	struct drm_connector_list_iter conn_list_iter;
> > +	struct drm_crtc_state *crtc_state;
> > +
> > +	drm_connector_list_iter_begin(&dev_priv->drm, &conn_list_iter);
> > +	drm_for_each_connector_iter(conn_iter, &conn_list_iter) {
> > +		struct drm_connector_state *conn_iter_state;
> > +
> > +		if (!conn_iter->has_tile)
> > +			continue;
> > +		conn_iter_state = drm_atomic_get_connector_state(&state->base,
> > +								 conn_iter);
> > +		if (IS_ERR(conn_iter_state)) {
> > +			drm_connector_list_iter_end(&conn_list_iter);
> > +			return PTR_ERR(conn_iter_state);
> > +		}
> > +
> > +		if (!conn_iter_state->crtc)
> > +			continue;
> > +
> > +		if (conn_iter->tile_group->id != tile_grp_id)
> > +			continue;
> > +
> > +		crtc_state = drm_atomic_get_crtc_state(&state->base, conn_iter_state->crtc);
> > +		if (IS_ERR(crtc_state)) {
> > +			drm_connector_list_iter_end(&conn_list_iter);
> > +			return PTR_ERR(conn_iter_state);
> > +		}
> > +		crtc_state->mode_changed = true;
> > +	}
> > +	drm_connector_list_iter_end(&conn_list_iter);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +intel_dp_atomic_trans_port_sync_check(struct drm_i915_private *dev_priv,
> > +				      struct intel_atomic_state *state)
> > +{
> > +	struct drm_connector *connector;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_connector_state *connector_state;
> > +	int i, ret, tile_grp_id = 0;
> > +
> > +	if (INTEL_GEN(dev_priv) < 11)
> > +		return 0;
> > +
> > +	/* Is tiled, mark all other tiled CRTCs as needing a modeset */
> > +	for_each_new_connector_in_state(&state->base, connector, connector_state, i) {
> > +		if (!connector->has_tile)
> > +			continue;
> > +		if (connector_state->crtc &&
> > +		    tile_grp_id != connector->tile_group->id) {
> > +			crtc_state = drm_atomic_get_new_crtc_state(&state->base,
> > +								   connector_state->crtc);
> > +			if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > +				continue;
> > +
> > +			tile_grp_id = connector->tile_group->id;
> > +		} else
> > +			continue;
> > +
> > +		ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> BTW after some more pondering I don't think this alone is sufficient.
> The tile information may have already disppeared so I believe we also
> need to make sure we mark all currently synced crtcs as needing a
> modeset if any of them need a modeset. And I guess that's pretty much
> the same function we'll need to handle fastset correctly.
>

The crtcs in the current state get synced and master/slave assignments happen in icl_add_sync_mode_crtcs before compute_config call
So are you suggesting basically moving this function after the crtcs are synced and then just setting modeset
to true for all synced crtcs if one of them needs modeset?

And why would the tile information be disappeared?  

Manasi

> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -14093,6 +14167,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >  
> > +	ret = intel_dp_atomic_trans_port_sync_check(dev_priv, state);
> > +	if (ret)
> > +		goto fail;
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state)) {
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx