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

Commit Message

Navare, Manasi Dec. 11, 2019, 9:14 p.m. UTC

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ä <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: José Roberto de Souza <[e-mail]>
Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Signed-off-by: Manasi Navare <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_display.c | 78 ++++++++++++++++++++
 1 file changed, 78 insertions(+)

Comments

Matt Roper Dec. 13, 2019, 12:32 a.m. UTC | #1

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

Minor typo on "linked to" here.

> 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ä <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: José Roberto de Souza <[e-mail]>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5

I think we're moving to "Closes:" as the annotation here now that it's
not actually a bugzilla bug database anymore.

> Signed-off-by: Manasi Navare <[e-mail]>
> ---
>  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

Minor kernel coding style violation; if we use {} on one branch of an
if, we need to use them on all.

> +			continue;
> +
> +		ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * 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;

Should this happen before the drm_atomic_helper_check_modeset() just
above (or should we re-call that function if we flag the other tile as
needing a modeset)?  The kerneldoc on that function says:

"""
Drivers which set &drm_crtc_state.mode_changed [...] _must_ call this
function afterwards after that change. It is permitted to call this
function multiple times for the same update ...
"""


Matt

> +
>  	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
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Dec. 13, 2019, 1:18 a.m. UTC | #2

On Thu, Dec 12, 2019 at 04:32:32PM -0800, Matt Roper 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
> 
> Minor typo on "linked to" here.

I will fix it

> 
> > 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ä <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: José Roberto de Souza <[e-mail]>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> 
> I think we're moving to "Closes:" as the annotation here now that it's
> not actually a bugzilla bug database anymore.

Ok cool, will change that to Closes

> 
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  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
> 
> Minor kernel coding style violation; if we use {} on one branch of an
> if, we need to use them on all.
>

Yes i got a checkpatch check warning, will fix it
 
> > +			continue;
> > +
> > +		ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * 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;
> 
> Should this happen before the drm_atomic_helper_check_modeset() just
> above (or should we re-call that function if we flag the other tile as
> needing a modeset)?  The kerneldoc on that function says:
> 
> """
> Drivers which set &drm_crtc_state.mode_changed [...] _must_ call this
> function afterwards after that change. It is permitted to call this
> function multiple times for the same update ...
> """
>

IMO, here infact it makes sense to call my function after the drm_atomic_helper_check_modeset()
because it directly sets the new_crtc_state->mode_changed to true for all tiles if 1 of them needs
a full modeset.
And whether that one tile needs a full modeset or not will be decided based on mode changed for that set
in drm_atomic_helper_check_modeset.

Manasi
 
> 
> Matt
> 
> > +
> >  	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
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> [phone]

Matt Roper Dec. 13, 2019, 3:11 a.m. UTC | #3

On Thu, Dec 12, 2019 at 05:18:02PM -0800, Manasi Navare wrote:
> On Thu, Dec 12, 2019 at 04:32:32PM -0800, Matt Roper 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
> > 
> > Minor typo on "linked to" here.
> 
> I will fix it
> 
> > 
> > > 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ä <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: José Roberto de Souza <[e-mail]>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > 
> > I think we're moving to "Closes:" as the annotation here now that it's
> > not actually a bugzilla bug database anymore.
> 
> Ok cool, will change that to Closes
> 
> > 
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > ---
> > >  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
> > 
> > Minor kernel coding style violation; if we use {} on one branch of an
> > if, we need to use them on all.
> >
> 
> Yes i got a checkpatch check warning, will fix it
>  
> > > +			continue;
> > > +
> > > +		ret = intel_dp_modeset_all_tiles(dev_priv, state, tile_grp_id);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * 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;
> > 
> > Should this happen before the drm_atomic_helper_check_modeset() just
> > above (or should we re-call that function if we flag the other tile as
> > needing a modeset)?  The kerneldoc on that function says:
> > 
> > """
> > Drivers which set &drm_crtc_state.mode_changed [...] _must_ call this
> > function afterwards after that change. It is permitted to call this
> > function multiple times for the same update ...
> > """
> >
> 
> IMO, here infact it makes sense to call my function after the drm_atomic_helper_check_modeset()
> because it directly sets the new_crtc_state->mode_changed to true for all tiles if 1 of them needs
> a full modeset.
> And whether that one tile needs a full modeset or not will be decided based on mode changed for that set
> in drm_atomic_helper_check_modeset.

Okay, makes sense.  But based on the comment, I believe we still need to
call the helper a second time during/after your function in the event
that we've switched any additional crtcs to mode_changed=true.

I'm also wondering whether we should eventually move most of your logic
here out of i915 and into a general drm helper that other drivers can
utilize too.  But I'm okay with doing that as a followup after we've
landed it and verified it as an i915-specific change first.


Matt


> 
> Manasi
>  
> > 
> > Matt
> > 
> > > +
> > >  	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
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > VTT-OSGC Platform Enablement
> > Intel Corporation
> > [phone]

Ville Syrjala Dec. 13, 2019, 8:05 p.m. UTC | #4

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ä <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: José Roberto de Souza <[e-mail]>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <[e-mail]>
> ---
>  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;
'connector'
> +	struct drm_connector_list_iter conn_list_iter;
> +	struct drm_crtc_state *crtc_state;

crtc_state has needlessly wide scope.

> +
> +	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;

'conn_state' is the most popular name.

> +
> +		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;

The tile group check should be part of the same if with the has_tile
check.

> +
> +		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,

Pointless variable. Can be extracted from the atomic state.

> +				      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;

tile_grp_id is rather pointless. crtc_state and ret can move into
tighter scope. And the next suggestion allows you to kill crtc_state
entirely...

> +
> +	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;

This should to be able to be shortened to just intel_connector_needs_modeset().

> +
> +			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;
> +}
> +
>  /**
>   * 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;

We should probably do this from the connector .atomic_check() hook if
Jose is going to do the MST thing that way. But no real problem doing
here I think.

> +
>  	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

Navare, Manasi Dec. 13, 2019, 9:05 p.m. UTC | #5

On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: José Roberto de Souza <[e-mail]>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  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;
> 'connector'

Ok

> > +	struct drm_connector_list_iter conn_list_iter;
> > +	struct drm_crtc_state *crtc_state;
> 
> crtc_state has needlessly wide scope.

Ok will add it inside the loop

> 
> > +
> > +	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;
> 
> 'conn_state' is the most popular name.
> 
> > +
> > +		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;
> 
> The tile group check should be part of the same if with the has_tile
> check.

Ok yes. So not pass the tile grp id at all just compare with current
connector's tile_grp_id ?

> 
> > +
> > +		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,
> 
> Pointless variable. Can be extracted from the atomic state.
> 
> > +				      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;
> 
> tile_grp_id is rather pointless. crtc_state and ret can move into
> tighter scope. And the next suggestion allows you to kill crtc_state
> entirely...

tile_grp_id  is useless because I should just use it from current connector obtained from state?

> 
> > +
> > +	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;
> 
> This should to be able to be shortened to just intel_connector_needs_modeset().

Ok will use that instead.

> 
> > +
> > +			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;
> > +}
> > +
> >  /**
> >   * 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;
> 
> We should probably do this from the connector .atomic_check() hook if
> Jose is going to do the MST thing that way. But no real problem doing
> here I think.

If you dont have a strong preference I will leave it here since Jose's code is not in yet.

Manasi

> 
> > +
> >  	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

Ville Syrjala Dec. 13, 2019, 9:17 p.m. UTC | #6

On Fri, Dec 13, 2019 at 01:05:48PM -0800, Manasi Navare wrote:
> On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: José Roberto de Souza <[e-mail]>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > ---
> > >  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;
> > 'connector'
> 
> Ok
> 
> > > +	struct drm_connector_list_iter conn_list_iter;
> > > +	struct drm_crtc_state *crtc_state;
> > 
> > crtc_state has needlessly wide scope.
> 
> Ok will add it inside the loop
> 
> > 
> > > +
> > > +	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;
> > 
> > 'conn_state' is the most popular name.
> > 
> > > +
> > > +		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;
> > 
> > The tile group check should be part of the same if with the has_tile
> > check.
> 
> Ok yes. So not pass the tile grp id at all just compare with current
> connector's tile_grp_id ?

You still need to pass in the tile_grp_id from the caller's current
connector. What I meant is that you should do 

if (!connector->has_tile || connector->tile_grp != tile_grp)
	continue;

before you do the drm_atomic_get_connector_state(). We don't want to
needlessly add connectors belonging to some other tile group to our
atomic state.

> 
> > 
> > > +
> > > +		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,
> > 
> > Pointless variable. Can be extracted from the atomic state.
> > 
> > > +				      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;
> > 
> > tile_grp_id is rather pointless. crtc_state and ret can move into
> > tighter scope. And the next suggestion allows you to kill crtc_state
> > entirely...
> 
> tile_grp_id  is useless because I should just use it from current connector obtained from state?

I just meant that you use that variable exactly once. So instead you can
just directly do:

ret = intel_dp_modeset_all_tiles(state, connector->tile_grp_id);
...



> 
> > 
> > > +
> > > +	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;
> > 
> > This should to be able to be shortened to just intel_connector_needs_modeset().
> 
> Ok will use that instead.
> 
> > 
> > > +
> > > +			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;
> > > +}
> > > +
> > >  /**
> > >   * 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;
> > 
> > We should probably do this from the connector .atomic_check() hook if
> > Jose is going to do the MST thing that way. But no real problem doing
> > here I think.
> 
> If you dont have a strong preference I will leave it here since Jose's code is not in yet.
> 
> Manasi
> 
> > 
> > > +
> > >  	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

Navare, Manasi Dec. 14, 2019, 2:28 a.m. UTC | #7

On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: José Roberto de Souza <[e-mail]>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  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;
> 'connector'
> > +	struct drm_connector_list_iter conn_list_iter;
> > +	struct drm_crtc_state *crtc_state;
> 
> crtc_state has needlessly wide scope.
> 
> > +
> > +	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;
> 
> 'conn_state' is the most popular name.
> 
> > +
> > +		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;
> 
> The tile group check should be part of the same if with the has_tile
> check.
> 
> > +
> > +		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,
> 
> Pointless variable. Can be extracted from the atomic state.
> 
> > +				      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;
> 
> tile_grp_id is rather pointless. crtc_state and ret can move into
> tighter scope. And the next suggestion allows you to kill crtc_state
> entirely...

Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
and I make sure that I dont enter into the loop to check modeset again for the connector with
same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()

How can I achieve this instead?

Manasi

> 
> > +
> > +	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;
> 
> This should to be able to be shortened to just intel_connector_needs_modeset().
> 
> > +
> > +			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;
> > +}
> > +
> >  /**
> >   * 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;
> 
> We should probably do this from the connector .atomic_check() hook if
> Jose is going to do the MST thing that way. But no real problem doing
> here I think.
> 
> > +
> >  	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

Ville Syrjala Dec. 16, 2019, 12:03 p.m. UTC | #8

On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: José Roberto de Souza <[e-mail]>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > ---
> > >  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;
> > 'connector'
> > > +	struct drm_connector_list_iter conn_list_iter;
> > > +	struct drm_crtc_state *crtc_state;
> > 
> > crtc_state has needlessly wide scope.
> > 
> > > +
> > > +	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;
> > 
> > 'conn_state' is the most popular name.
> > 
> > > +
> > > +		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;
> > 
> > The tile group check should be part of the same if with the has_tile
> > check.
> > 
> > > +
> > > +		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,
> > 
> > Pointless variable. Can be extracted from the atomic state.
> > 
> > > +				      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;
> > 
> > tile_grp_id is rather pointless. crtc_state and ret can move into
> > tighter scope. And the next suggestion allows you to kill crtc_state
> > entirely...
> 
> Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> and I make sure that I dont enter into the loop to check modeset again for the connector with
> same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> 
> How can I achieve this instead?


Instead of 
foo()
{
	tile_grp_id = 0;

	for_each() {
		tile_grp_id = conn->tile_grp_id;

		intel_dp_modeset_all_tiles(tile_grp_id);
	}
}

you just do
foo()
{
	for_each() {
		intel_dp_modeset_all_tiles(conn->tile_grp_id);
	}
}

Ville Syrjala Dec. 16, 2019, 2:37 p.m. UTC | #9

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ä <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: José Roberto de Souza <[e-mail]>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Signed-off-by: Manasi Navare <[e-mail]>
> ---
>  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.

> +
>  /**
>   * 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

Navare, Manasi Dec. 16, 2019, 4:40 p.m. UTC | #10

On Mon, Dec 16, 2019 at 02:03:43PM +0200, Ville Syrjälä wrote:
> On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> > On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Cc: José Roberto de Souza <[e-mail]>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > ---
> > > >  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;
> > > 'connector'
> > > > +	struct drm_connector_list_iter conn_list_iter;
> > > > +	struct drm_crtc_state *crtc_state;
> > > 
> > > crtc_state has needlessly wide scope.
> > > 
> > > > +
> > > > +	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;
> > > 
> > > 'conn_state' is the most popular name.
> > > 
> > > > +
> > > > +		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;
> > > 
> > > The tile group check should be part of the same if with the has_tile
> > > check.
> > > 
> > > > +
> > > > +		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,
> > > 
> > > Pointless variable. Can be extracted from the atomic state.
> > > 
> > > > +				      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;
> > > 
> > > tile_grp_id is rather pointless. crtc_state and ret can move into
> > > tighter scope. And the next suggestion allows you to kill crtc_state
> > > entirely...
> > 
> > Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> > and I make sure that I dont enter into the loop to check modeset again for the connector with
> > same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> > 
> > How can I achieve this instead?
> 
> 
> Instead of 
> foo()
> {
> 	tile_grp_id = 0;
> 
> 	for_each() {
> 		tile_grp_id = conn->tile_grp_id;
> 
> 		intel_dp_modeset_all_tiles(tile_grp_id);
> 	}
> }
> 
> you just do
> foo()
> {
> 	for_each() {
> 		intel_dp_modeset_all_tiles(conn->tile_grp_id);
> 	}
> }

Yes I understand that we can pass the conn->tile_grp_id directly. But currently I am using tile_grp_id in for_each(), to call intel_dp_modeset_all_tiles()
only if that cnnector is from a differnt tile grp id else continue.

foo()
{
	for_each() {
		if(tile_grp_id != conn->tile_grp_id)
			intel_dpmodeset_all_tiles();
		else
			continue;
	}
}

calling intel_dp_modeset_all_tiles() for all tiles is kind of redundant. How can I do this with your suggestion, or do you think
we should call intel_dp_modeset_all_tiles for each connector anyway and it shouldnt matter?

Regards
Manasi
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjala Dec. 16, 2019, 5:11 p.m. UTC | #11

On Mon, Dec 16, 2019 at 08:40:24AM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 02:03:43PM +0200, Ville Syrjälä wrote:
> > On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> > > On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > Cc: José Roberto de Souza <[e-mail]>
> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > > ---
> > > > >  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;
> > > > 'connector'
> > > > > +	struct drm_connector_list_iter conn_list_iter;
> > > > > +	struct drm_crtc_state *crtc_state;
> > > > 
> > > > crtc_state has needlessly wide scope.
> > > > 
> > > > > +
> > > > > +	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;
> > > > 
> > > > 'conn_state' is the most popular name.
> > > > 
> > > > > +
> > > > > +		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;
> > > > 
> > > > The tile group check should be part of the same if with the has_tile
> > > > check.
> > > > 
> > > > > +
> > > > > +		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,
> > > > 
> > > > Pointless variable. Can be extracted from the atomic state.
> > > > 
> > > > > +				      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;
> > > > 
> > > > tile_grp_id is rather pointless. crtc_state and ret can move into
> > > > tighter scope. And the next suggestion allows you to kill crtc_state
> > > > entirely...
> > > 
> > > Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> > > and I make sure that I dont enter into the loop to check modeset again for the connector with
> > > same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> > > 
> > > How can I achieve this instead?
> > 
> > 
> > Instead of 
> > foo()
> > {
> > 	tile_grp_id = 0;
> > 
> > 	for_each() {
> > 		tile_grp_id = conn->tile_grp_id;
> > 
> > 		intel_dp_modeset_all_tiles(tile_grp_id);
> > 	}
> > }
> > 
> > you just do
> > foo()
> > {
> > 	for_each() {
> > 		intel_dp_modeset_all_tiles(conn->tile_grp_id);
> > 	}
> > }
> 
> Yes I understand that we can pass the conn->tile_grp_id directly. But currently I am using tile_grp_id in for_each(), to call intel_dp_modeset_all_tiles()
> only if that cnnector is from a differnt tile grp id else continue.
> 
> foo()
> {
> 	for_each() {
> 		if(tile_grp_id != conn->tile_grp_id)
> 			intel_dpmodeset_all_tiles();
> 		else
> 			continue;
> 	}
> }
> 
> calling intel_dp_modeset_all_tiles() for all tiles is kind of redundant. How can I do this with your suggestion, or do you think
> we should call intel_dp_modeset_all_tiles for each connector anyway and it shouldnt matter?

Yes if that connector is tiled.

Eg. if you have something like
connector A: tile group 1
connector B: tile group 2
connector C: tile group 3
connector D: tile group 2

then your single tile_grp_id variable doesn't work anyway. You'd need
track which tile groups you've already handled and skip those. But
much easier to just blindly plow ahead.

Navare, Manasi Dec. 16, 2019, 7:13 p.m. UTC | #12

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ä <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: José Roberto de Souza <[e-mail]>
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Signed-off-by: Manasi Navare <[e-mail]>
> > ---
> >  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

Ville Syrjala Dec. 16, 2019, 9:37 p.m. UTC | #13

On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> 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ä <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: José Roberto de Souza <[e-mail]>
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > ---
> > >  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?

I think it should look something like:

modeset_tiled_things();
modeset_synced_things();

for_each() {
	modeset_pipes_config();
	if (can_fastset()) {
		modeset=false;
		fastset=true;
	}
}

modeset_synced_things();

for_each() {
	if (!modeset && fastset)
		copy_state();
}

> 
> And why would the tile information be disappeared?  

It'll get updated whenever someone does a getconnector() or whatever.

Example:
1. sync pipe A and B, pipe A is master
2. swap pipe B display for something else
3. getconnector() -> tile info goes poof
4. do something on pipe A that needs a modeset
   no tile info so we miss that pipe B also needs a modeset

Navare, Manasi Dec. 16, 2019, 9:42 p.m. UTC | #14

On Mon, Dec 16, 2019 at 07:11:20PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 08:40:24AM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 02:03:43PM +0200, Ville Syrjälä wrote:
> > > On Fri, Dec 13, 2019 at 06:28:40PM -0800, Manasi Navare wrote:
> > > > On Fri, Dec 13, 2019 at 10:05:49PM +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ä <[e-mail]>
> > > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > > Cc: José Roberto de Souza <[e-mail]>
> > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > ---
> > > > > >  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;
> > > > > 'connector'
> > > > > > +	struct drm_connector_list_iter conn_list_iter;
> > > > > > +	struct drm_crtc_state *crtc_state;
> > > > > 
> > > > > crtc_state has needlessly wide scope.
> > > > > 
> > > > > > +
> > > > > > +	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;
> > > > > 
> > > > > 'conn_state' is the most popular name.
> > > > > 
> > > > > > +
> > > > > > +		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;
> > > > > 
> > > > > The tile group check should be part of the same if with the has_tile
> > > > > check.
> > > > > 
> > > > > > +
> > > > > > +		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,
> > > > > 
> > > > > Pointless variable. Can be extracted from the atomic state.
> > > > > 
> > > > > > +				      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;
> > > > > 
> > > > > tile_grp_id is rather pointless. crtc_state and ret can move into
> > > > > tighter scope. And the next suggestion allows you to kill crtc_state
> > > > > entirely...
> > > > 
> > > > Its not clear why tile_grp_id is pointless, I am using tile_grp_id for the first connector with has_tile
> > > > and I make sure that I dont enter into the loop to check modeset again for the connector with
> > > > same tile_grp_id because we have already set its mode changed to true in intel_dp_modeset_all_tiles()
> > > > 
> > > > How can I achieve this instead?
> > > 
> > > 
> > > Instead of 
> > > foo()
> > > {
> > > 	tile_grp_id = 0;
> > > 
> > > 	for_each() {
> > > 		tile_grp_id = conn->tile_grp_id;
> > > 
> > > 		intel_dp_modeset_all_tiles(tile_grp_id);
> > > 	}
> > > }
> > > 
> > > you just do
> > > foo()
> > > {
> > > 	for_each() {
> > > 		intel_dp_modeset_all_tiles(conn->tile_grp_id);
> > > 	}
> > > }
> > 
> > Yes I understand that we can pass the conn->tile_grp_id directly. But currently I am using tile_grp_id in for_each(), to call intel_dp_modeset_all_tiles()
> > only if that cnnector is from a differnt tile grp id else continue.
> > 
> > foo()
> > {
> > 	for_each() {
> > 		if(tile_grp_id != conn->tile_grp_id)
> > 			intel_dpmodeset_all_tiles();
> > 		else
> > 			continue;
> > 	}
> > }
> > 
> > calling intel_dp_modeset_all_tiles() for all tiles is kind of redundant. How can I do this with your suggestion, or do you think
> > we should call intel_dp_modeset_all_tiles for each connector anyway and it shouldnt matter?
> 
> Yes if that connector is tiled.
> 
> Eg. if you have something like
> connector A: tile group 1
> connector B: tile group 2
> connector C: tile group 3
> connector D: tile group 2
> 
> then your single tile_grp_id variable doesn't work anyway. You'd need
> track which tile groups you've already handled and skip those. But
> much easier to just blindly plow ahead.

Okay so in our case :
Conn A: tile gro 1
Conn B: tile grp 1

Now foo {

	for_each_conn {
		//connA
		intel_modeset_all_tiles() // this will set mode to true for conn B in iteration 1
	}
}
Now in the second iteration
 	for_each_conn {
		// conn B iter 2
		intel_modeset_all_tiles() // this will set mode to true for conn A 
	}
redundant second iteration since both are in same tile grp

Manasi

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

Navare, Manasi Dec. 16, 2019, 10:33 p.m. UTC | #15

On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > 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ä <[e-mail]>
> > > > Cc: Ville Syrjälä <[e-mail]>
> > > > Cc: José Roberto de Souza <[e-mail]>
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > ---
> > > >  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?
> 
> I think it should look something like:
> 
> modeset_tiled_things();
> modeset_synced_things();

but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
from within modeset_pipe_config just before compute_config() call.

> 
> for_each() {

This is the for_each_crtc loop in intel_atomic_check() right?

> 	modeset_pipes_config();

So have icl_add_sync_crtcs outside of modeset_pipe_config()?

> 	if (can_fastset()) {
> 		modeset=false;
> 		fastset=true;
> 	}
> }
> 
> modeset_synced_things();
> 
> for_each() {
> 	if (!modeset && fastset)
> 		copy_state();
> }
We already do this in the code right?

Manasi

> 
> > 
> > And why would the tile information be disappeared?  
> 
> It'll get updated whenever someone does a getconnector() or whatever.
> 
> Example:
> 1. sync pipe A and B, pipe A is master
> 2. swap pipe B display for something else
> 3. getconnector() -> tile info goes poof
> 4. do something on pipe A that needs a modeset
>    no tile info so we miss that pipe B also needs a modeset
> 
> -- 
> Ville Syrjälä
> Intel

Navare, Manasi Dec. 16, 2019, 10:58 p.m. UTC | #16

On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > 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ä <[e-mail]>
> > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > Cc: José Roberto de Souza <[e-mail]>
> > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > > ---
> > > > >  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?
> > 
> > I think it should look something like:
> > 
> > modeset_tiled_things();
> > modeset_synced_things();
> 
> but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> from within modeset_pipe_config just before compute_config() call.
> 
> > 
> > for_each() {
> 
> This is the for_each_crtc loop in intel_atomic_check() right?
> 
> > 	modeset_pipes_config();
> 
> So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> 
> > 	if (can_fastset()) {
> > 		modeset=false;
> > 		fastset=true;
> > 	}
> > }
> > 
> > modeset_synced_things();
> > 
> > for_each() {
> > 	if (!modeset && fastset)
> > 		copy_state();
> > }
> We already do this in the code right?
> 
> Manasi
> 
> > 
> > > 
> > > And why would the tile information be disappeared?  
> > 
> > It'll get updated whenever someone does a getconnector() or whatever.
> > 
> > Example:
> > 1. sync pipe A and B, pipe A is master
> > 2. swap pipe B display for something else

If we disconnect and connect other display for pipe B, port sync mode is off and
Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile

so why we wold need to add both pipes to modeset in this case at all

Manasi

> > 3. getconnector() -> tile info goes poof
> > 4. do something on pipe A that needs a modeset
> >    no tile info so we miss that pipe B also needs a modeset
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjala Dec. 17, 2019, 10:50 a.m. UTC | #17

On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > 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ä <[e-mail]>
> > > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > > Cc: José Roberto de Souza <[e-mail]>
> > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > ---
> > > > > >  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?
> > > 
> > > I think it should look something like:
> > > 
> > > modeset_tiled_things();
> > > modeset_synced_things();
> > 
> > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > from within modeset_pipe_config just before compute_config() call.
> > 
> > > 
> > > for_each() {
> > 
> > This is the for_each_crtc loop in intel_atomic_check() right?
> > 
> > > 	modeset_pipes_config();
> > 
> > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> > 
> > > 	if (can_fastset()) {
> > > 		modeset=false;
> > > 		fastset=true;
> > > 	}
> > > }
> > > 
> > > modeset_synced_things();
> > > 
> > > for_each() {
> > > 	if (!modeset && fastset)
> > > 		copy_state();
> > > }
> > We already do this in the code right?
> > 
> > Manasi
> > 
> > > 
> > > > 
> > > > And why would the tile information be disappeared?  
> > > 
> > > It'll get updated whenever someone does a getconnector() or whatever.
> > > 
> > > Example:
> > > 1. sync pipe A and B, pipe A is master
> > > 2. swap pipe B display for something else
> 
> If we disconnect and connect other display for pipe B, port sync mode is off and
> Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile

Port sync will stay enabled until we do a modeset to disable it. In this
example there is never any modeset on pipe B until we add soemthing to
force one in step 4.

> 
> so why we wold need to add both pipes to modeset in this case at all
> 
> Manasi
> 
> > > 3. getconnector() -> tile info goes poof
> > > 4. do something on pipe A that needs a modeset
> > >    no tile info so we miss that pipe B also needs a modeset
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi Dec. 17, 2019, 7:04 p.m. UTC | #18

On Tue, Dec 17, 2019 at 12:50:31PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> > On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > > 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ä <[e-mail]>
> > > > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > > > Cc: José Roberto de Souza <[e-mail]>
> > > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > ---
> > > > > > >  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?
> > > > 
> > > > I think it should look something like:
> > > > 
> > > > modeset_tiled_things();
> > > > modeset_synced_things();
> > > 
> > > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > > from within modeset_pipe_config just before compute_config() call.
> > > 
> > > > 
> > > > for_each() {
> > > 
> > > This is the for_each_crtc loop in intel_atomic_check() right?
> > > 
> > > > 	modeset_pipes_config();
> > > 
> > > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> > > 
> > > > 	if (can_fastset()) {
> > > > 		modeset=false;
> > > > 		fastset=true;
> > > > 	}
> > > > }
> > > > 
> > > > modeset_synced_things();
> > > > 
> > > > for_each() {
> > > > 	if (!modeset && fastset)
> > > > 		copy_state();
> > > > }
> > > We already do this in the code right?
> > > 
> > > Manasi
> > > 
> > > > 
> > > > > 
> > > > > And why would the tile information be disappeared?  
> > > > 
> > > > It'll get updated whenever someone does a getconnector() or whatever.
> > > > 
> > > > Example:
> > > > 1. sync pipe A and B, pipe A is master
> > > > 2. swap pipe B display for something else
> > 
> > If we disconnect and connect other display for pipe B, port sync mode is off and
> > Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
> 
> Port sync will stay enabled until we do a modeset to disable it. In this
> example there is never any modeset on pipe B until we add soemthing to
> force one in step 4.

seems to be working with the current intel_dp_add_modeset_tiles() since the moment
we unplug, it goes through the disable sequence where all the associated tiles get disabled
and port sync mode gets disabled. This happens because the Pipe A which is still connected now indicates
a mode change since it fallsback to a lower non tiled mode.

But to be on safer side, i can check if say Pipe A is a master or slave (in port sync mode) , check its crtc_state which is
still showing the old master slave links since we havent cleared those yet and then add all other synced crtcs
to the modeset?

So the order of calls can still be :
intel_atomic_check() {

intel_dp_atomic_tiled_check() { modeset_all_tiles modeset_synced_crtcs}
intel_modeset_pipe_Config()
icl_add_sync_crtcs
compute_config
fastsetcheck()
modeset_synced_things (here it looks at the new master slave assignments)

Does this look correct, I want send this patch out today

Manasi

> 
> > 
> > so why we wold need to add both pipes to modeset in this case at all
> > 
> > Manasi
> > 
> > > > 3. getconnector() -> tile info goes poof
> > > > 4. do something on pipe A that needs a modeset
> > > >    no tile info so we miss that pipe B also needs a modeset
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

Navare, Manasi Dec. 19, 2019, 2:37 a.m. UTC | #19

Hi Ville,

I double checke that I can use new_crtc_state in modeset_synced_crtcs to add all synced crtcs to modeset
That is working as expected so that the same function can be reused after fastset check to override
mode_changed to true.

Here's what I have:

static int                                                                                                                                                                                                                                                             
intel_dp_modeset_synced_crtcs(struct intel_atomic_state *state)                                                                                                                                                                                                        
{                                                                                                                                                                                                                                                                      
        struct drm_i915_private *dev_priv = to_i915(state->base.dev);                                                                                                                                                                                                  
        struct intel_crtc_state *old_crtc_state, *new_crtc_state;                                                                                                                                                                                                      
        struct intel_crtc *crtc;                                                                                                                                                                                                                                       
        int i;                                                                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                       
        for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,                                                                                                                                                                                               
                                            new_crtc_state, i) {                                                                                                                                                                                                       
                if(is_trans_port_sync_mode(new_crtc_state))                                                                                                                                                                                                            
                        DRM_DEBUG_KMS("\n Adding CRTC %d:%s to modeset since old crtc state master trans = %s, slave mask = %d, new crtc state master trans = %s slave mask = %d",                                                                                     
                                      crtc->base.base.id,                                                                                                                                                                                                              
                                      crtc->base.name,                                                                                                                                                                                                                 
                                      transcoder_name(old_crtc_state->master_transcoder),                                                                                                                                                                              
                                      old_crtc_state->sync_mode_slaves_mask,                                                                                                                                                                                           
                                      transcoder_name(new_crtc_state->master_transcoder),                                                                                                                                                                              
                                      new_crtc_state->sync_mode_slaves_mask);                                                                                                                                                                                          
                        new_crtc_state->uapi.mode_changed = true;                                                                                                                                                                                                      
        }                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                       
        return 0;                                                                                                                                                                                                                                                      
}


static int                                                                                                                                                                                                                                                             
intel_dp_atomic_check_synced_crtcs(struct intel_atomic_state *state)                                                                                                                                                                                                   
{                                                                                                                                                                                                                                                                      
        struct drm_i915_private *dev_priv = to_i915(state->base.dev);                                                                                                                                                                                                  
        struct intel_crtc_state *old_crtc_state, *new_crtc_state;                                                                                                                                                                                                      
        struct intel_crtc *crtc;                                                                                                                                                                                                                                       
        int i;                                                                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                       
        for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,                                                                                                                                                                                               
                                            new_crtc_state, i) {                                                                                                                                                                                                       
                if(!is_trans_port_sync_mode(new_crtc_state) ||                                                                                                                                                                                                         
                   !needs_modeset(new_crtc_state))                                                                                                                                                                                                                     
                        continue;                                                                                                                                                                                                                                      
                intel_dp_modeset_synced_crtcs(state);                                                                                                                                                                                                                  
        }                                                                                                                                                                                                                                                              
                                                                                                                                                                                                                                                                       
        return 0;                                                                                                                                                                                                                                                      
}

I will add this to this patch after thye call to modeset_all_tiles() and submit a new revision.

Regards
Manasi     




On Tue, Dec 17, 2019 at 11:04:53AM -0800, Manasi Navare wrote:
> On Tue, Dec 17, 2019 at 12:50:31PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 16, 2019 at 02:58:13PM -0800, Manasi Navare wrote:
> > > On Mon, Dec 16, 2019 at 02:33:10PM -0800, Manasi Navare wrote:
> > > > On Mon, Dec 16, 2019 at 11:37:38PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Dec 16, 2019 at 11:13:09AM -0800, Manasi Navare wrote:
> > > > > > 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ä <[e-mail]>
> > > > > > > > Cc: Ville Syrjälä <[e-mail]>
> > > > > > > > Cc: José Roberto de Souza <[e-mail]>
> > > > > > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > > > > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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?
> > > > > 
> > > > > I think it should look something like:
> > > > > 
> > > > > modeset_tiled_things();
> > > > > modeset_synced_things();
> > > > 
> > > > but modeset_synced_things() should be called after icl_add_sync_crtcs() which as per your review should be called
> > > > from within modeset_pipe_config just before compute_config() call.
> > > > 
> > > > > 
> > > > > for_each() {
> > > > 
> > > > This is the for_each_crtc loop in intel_atomic_check() right?
> > > > 
> > > > > 	modeset_pipes_config();
> > > > 
> > > > So have icl_add_sync_crtcs outside of modeset_pipe_config()?
> > > > 
> > > > > 	if (can_fastset()) {
> > > > > 		modeset=false;
> > > > > 		fastset=true;
> > > > > 	}
> > > > > }
> > > > > 
> > > > > modeset_synced_things();
> > > > > 
> > > > > for_each() {
> > > > > 	if (!modeset && fastset)
> > > > > 		copy_state();
> > > > > }
> > > > We already do this in the code right?
> > > > 
> > > > Manasi
> > > > 
> > > > > 
> > > > > > 
> > > > > > And why would the tile information be disappeared?  
> > > > > 
> > > > > It'll get updated whenever someone does a getconnector() or whatever.
> > > > > 
> > > > > Example:
> > > > > 1. sync pipe A and B, pipe A is master
> > > > > 2. swap pipe B display for something else
> > > 
> > > If we disconnect and connect other display for pipe B, port sync mode is off and
> > > Pipe A no longer a master and we would reset the master_slave assignments and conn on pipe B would not have tile
> > 
> > Port sync will stay enabled until we do a modeset to disable it. In this
> > example there is never any modeset on pipe B until we add soemthing to
> > force one in step 4.
> 
> seems to be working with the current intel_dp_add_modeset_tiles() since the moment
> we unplug, it goes through the disable sequence where all the associated tiles get disabled
> and port sync mode gets disabled. This happens because the Pipe A which is still connected now indicates
> a mode change since it fallsback to a lower non tiled mode.
> 
> But to be on safer side, i can check if say Pipe A is a master or slave (in port sync mode) , check its crtc_state which is
> still showing the old master slave links since we havent cleared those yet and then add all other synced crtcs
> to the modeset?
> 
> So the order of calls can still be :
> intel_atomic_check() {
> 
> intel_dp_atomic_tiled_check() { modeset_all_tiles modeset_synced_crtcs}
> intel_modeset_pipe_Config()
> icl_add_sync_crtcs
> compute_config
> fastsetcheck()
> modeset_synced_things (here it looks at the new master slave assignments)
> 
> Does this look correct, I want send this patch out today
> 
> Manasi
> 
> > 
> > > 
> > > so why we wold need to add both pipes to modeset in this case at all
> > > 
> > > Manasi
> > > 
> > > > > 3. getconnector() -> tile info goes poof
> > > > > 4. do something on pipe A that needs a modeset
> > > > >    no tile info so we miss that pipe B also needs a modeset
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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;
+}
+
 /**
  * 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)) {