| Message ID | 20230914192659.757475-19-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Improve BW management on shared display links | expand |
On Thu, Sep 14, 2023 at 10:26:52PM +0300, Imre Deak wrote: > If an MST stream is modeset, its state must be checked along all the > other streams on the same MST link, for instance to resolve a BW > overallocation of a non-sink MST port or to make sure that the FEC is > enabled/disabled the same way for all these streams. > > To prepare for that this patch adds all the stream CRTCs to the atomic > state and marks them for modeset similarly to tgl+ platforms. (If the > state computation doesn't change the state the CRTC is switched back to > fastset mode.) > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index c1fea894d3774..832e8b0e87e84 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -491,9 +491,6 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector, > struct intel_connector *connector_iter; > int ret = 0; > > - if (DISPLAY_VER(dev_priv) < 12) > - return 0; > - I'm just a bit concerned, why this check was initially added? Probably there was a reason? Stan > if (!intel_connector_needs_modeset(state, &connector->base)) > return 0; > > -- > 2.37.2 >
On Wed, Sep 20, 2023 at 12:11:58PM +0300, Lisovskiy, Stanislav wrote: > On Thu, Sep 14, 2023 at 10:26:52PM +0300, Imre Deak wrote: > > If an MST stream is modeset, its state must be checked along all the > > other streams on the same MST link, for instance to resolve a BW > > overallocation of a non-sink MST port or to make sure that the FEC is > > enabled/disabled the same way for all these streams. > > > > To prepare for that this patch adds all the stream CRTCs to the atomic > > state and marks them for modeset similarly to tgl+ platforms. (If the > > state computation doesn't change the state the CRTC is switched back to > > fastset mode.) > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > index c1fea894d3774..832e8b0e87e84 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > @@ -491,9 +491,6 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector, > > struct intel_connector *connector_iter; > > int ret = 0; > > > > - if (DISPLAY_VER(dev_priv) < 12) > > - return 0; > > - > > I'm just a bit concerned, why this check was initially added? > Probably there was a reason? It's in the name of the function, which should be renamed if we're extending it beyond its original purpose. > > Stan > > > if (!intel_connector_needs_modeset(state, &connector->base)) > > return 0; > > > > -- > > 2.37.2 > >
On Wed, Sep 20, 2023 at 01:59:53PM +0300, Ville Syrjälä wrote: > On Wed, Sep 20, 2023 at 12:11:58PM +0300, Lisovskiy, Stanislav wrote: > > On Thu, Sep 14, 2023 at 10:26:52PM +0300, Imre Deak wrote: > > > If an MST stream is modeset, its state must be checked along all the > > > other streams on the same MST link, for instance to resolve a BW > > > overallocation of a non-sink MST port or to make sure that the FEC is > > > enabled/disabled the same way for all these streams. > > > > > > To prepare for that this patch adds all the stream CRTCs to the atomic > > > state and marks them for modeset similarly to tgl+ platforms. (If the > > > state computation doesn't change the state the CRTC is switched back to > > > fastset mode.) > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > index c1fea894d3774..832e8b0e87e84 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > @@ -491,9 +491,6 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector, > > > struct intel_connector *connector_iter; > > > int ret = 0; > > > > > > - if (DISPLAY_VER(dev_priv) < 12) > > > - return 0; > > > - > > > > I'm just a bit concerned, why this check was initially added? > > Probably there was a reason? > > It's in the name of the function, which should be renamed if we're > extending it beyond its original purpose. Well, I would say this check could be "a bit" more descriptive. Still, even if function name gets changed, we just remove the check, for the function which was initially not even intended to be called for pre-tgl? Why it became suitable now then? Or was it just wrong check? Stan > > > > > Stan > > > > > if (!intel_connector_needs_modeset(state, &connector->base)) > > > return 0; > > > > > > -- > > > 2.37.2 > > > > > -- > Ville Syrjälä > Intel
On Wed, Sep 20, 2023 at 02:25:14PM +0300, Lisovskiy, Stanislav wrote: > On Wed, Sep 20, 2023 at 01:59:53PM +0300, Ville Syrjälä wrote: > > On Wed, Sep 20, 2023 at 12:11:58PM +0300, Lisovskiy, Stanislav wrote: > > > On Thu, Sep 14, 2023 at 10:26:52PM +0300, Imre Deak wrote: > > > > If an MST stream is modeset, its state must be checked along all the > > > > other streams on the same MST link, for instance to resolve a BW > > > > overallocation of a non-sink MST port or to make sure that the FEC is > > > > enabled/disabled the same way for all these streams. > > > > > > > > To prepare for that this patch adds all the stream CRTCs to the atomic > > > > state and marks them for modeset similarly to tgl+ platforms. (If the > > > > state computation doesn't change the state the CRTC is switched back to > > > > fastset mode.) > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 --- > > > > 1 file changed, 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > index c1fea894d3774..832e8b0e87e84 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > @@ -491,9 +491,6 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector, > > > > struct intel_connector *connector_iter; > > > > int ret = 0; > > > > > > > > - if (DISPLAY_VER(dev_priv) < 12) > > > > - return 0; > > > > - > > > > > > I'm just a bit concerned, why this check was initially added? > > > Probably there was a reason? > > > > It's in the name of the function, which should be renamed if we're > > extending it beyond its original purpose. > > Well, I would say this check could be "a bit" more descriptive. > Still, even if function name gets changed, we just remove the check, for the > function which was initially not even intended to be called for pre-tgl? The change on tgl+ platforms is that all the MST streams go through a master transcoder, while on old platforms each stream has only its own transcoder. Hence on tgl+ all streams/CRTCs may need to be modeset, at least in the case the CRTC owning the master transcoder (for instance pipe A/transcoder A) is modeset. So on tgl+ here all CRTCs for a given MST topology is added to the state/marked for modeset. pre-tgl this wasn't necessary, until the need to recalculate the BW requirement of each stream came up, as described in the commit log. I can clarify the above in the commit log and rename the function accordingly, is that ok? > Why it became suitable now then? Or was it just wrong check? > > Stan > > > > > > > > > Stan > > > > > > > if (!intel_connector_needs_modeset(state, &connector->base)) > > > > return 0; > > > > > > > > -- > > > > 2.37.2 > > > > > > > > -- > > Ville Syrjälä > > Intel
On Wed, Sep 20, 2023 at 03:38:05PM +0300, Imre Deak wrote: > On Wed, Sep 20, 2023 at 02:25:14PM +0300, Lisovskiy, Stanislav wrote: > > On Wed, Sep 20, 2023 at 01:59:53PM +0300, Ville Syrjälä wrote: > > > On Wed, Sep 20, 2023 at 12:11:58PM +0300, Lisovskiy, Stanislav wrote: > > > > On Thu, Sep 14, 2023 at 10:26:52PM +0300, Imre Deak wrote: > > > > > If an MST stream is modeset, its state must be checked along all the > > > > > other streams on the same MST link, for instance to resolve a BW > > > > > overallocation of a non-sink MST port or to make sure that the FEC is > > > > > enabled/disabled the same way for all these streams. > > > > > > > > > > To prepare for that this patch adds all the stream CRTCs to the atomic > > > > > state and marks them for modeset similarly to tgl+ platforms. (If the > > > > > state computation doesn't change the state the CRTC is switched back to > > > > > fastset mode.) > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 --- > > > > > 1 file changed, 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > index c1fea894d3774..832e8b0e87e84 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > > > > > @@ -491,9 +491,6 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector, > > > > > struct intel_connector *connector_iter; > > > > > int ret = 0; > > > > > > > > > > - if (DISPLAY_VER(dev_priv) < 12) > > > > > - return 0; > > > > > - > > > > > > > > I'm just a bit concerned, why this check was initially added? > > > > Probably there was a reason? > > > > > > It's in the name of the function, which should be renamed if we're > > > extending it beyond its original purpose. > > > > Well, I would say this check could be "a bit" more descriptive. > > Still, even if function name gets changed, we just remove the check, for the > > function which was initially not even intended to be called for pre-tgl? > > The change on tgl+ platforms is that all the MST streams go through a > master transcoder, while on old platforms each stream has only its own > transcoder. Hence on tgl+ all streams/CRTCs may need to be modeset, at > least in the case the CRTC owning the master transcoder (for instance > pipe A/transcoder A) is modeset. So on tgl+ here all CRTCs for a given > MST topology is added to the state/marked for modeset. > > pre-tgl this wasn't necessary, until the need to recalculate the BW > requirement of each stream came up, as described in the commit log. > > I can clarify the above in the commit log and rename the function > accordingly, is that ok? Yes, thank you for good explanation. I would may be also add some note to actual function also, but up to you really. Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > Why it became suitable now then? Or was it just wrong check? > > > > Stan > > > > > > > > > > > > > Stan > > > > > > > > > if (!intel_connector_needs_modeset(state, &connector->base)) > > > > > return 0; > > > > > > > > > > -- > > > > > 2.37.2 > > > > > > > > > > > -- > > > Ville Syrjälä > > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index c1fea894d3774..832e8b0e87e84 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -491,9 +491,6 @@ intel_dp_mst_atomic_master_trans_check(struct intel_connector *connector, struct intel_connector *connector_iter; int ret = 0; - if (DISPLAY_VER(dev_priv) < 12) - return 0; - if (!intel_connector_needs_modeset(state, &connector->base)) return 0;
If an MST stream is modeset, its state must be checked along all the other streams on the same MST link, for instance to resolve a BW overallocation of a non-sink MST port or to make sure that the FEC is enabled/disabled the same way for all these streams. To prepare for that this patch adds all the stream CRTCs to the atomic state and marks them for modeset similarly to tgl+ platforms. (If the state computation doesn't change the state the CRTC is switched back to fastset mode.) Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 --- 1 file changed, 3 deletions(-)