Re: [PATCH v8 16/17] drm/dp_mst: Add helper to trigger modeset on affected DSC MST CRTCs

On Tue, 2019-12-03 at 09:35 -0500, mikita.lipski@amd.com wrote:

From: Mikita Lipski mikita.lipski@amd.com

[why] Whenever a connector on an MST network is changed or undergoes a modeset, the DSC configs for each stream on that topology will be recalculated. This can change their required bandwidth, requiring a full reprogramming, as though a modeset was performed, even if that stream did not change timing.

[how] Adding helper to trigger modesets on MST DSC connectors by setting mode_changed flag on CRTCs in the same topology as affected connector

v2: use drm_dp_mst_dsc_aux_for_port function to verify if the port is DSC capable

Cc: Manasi Navare manasi.d.navare@intel.com Cc: Lyude Paul lyude@redhat.com Signed-off-by: Mikita Lipski mikita.lipski@amd.com

---

drivers/gpu/drm/drm_dp_mst_topology.c | 62 +++++++++++++++++++++++++++ include/drm/drm_dp_mst_helper.h | 2 + 2 files changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 76bcbb4cd8b4..fb3710b727cc 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4802,6 +4802,68 @@ drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr, return 0; }

+/**

• • drm_dp_mst_add_affected_dsc_crtcs
• • @state: Pointer to the new &struct drm_dp_mst_topology_state
• • @port: Pointer tothe port of connector with new state
• • Whenever there is a change in mst topology
• • DSC configuration would have to be recalculated
• • therefore we need to trigger modeset on all affected
• • CRTCs in that topology
• • See also:
• • drm_dp_mst_atomic_enable_dsc()
• */

+int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state, struct drm_dp_mst_topology_mgr *mgr) +{

• struct drm_dp_mst_topology_state *mst_state;
• struct drm_dp_vcpi_allocation *pos;
• struct drm_connector *connector;
• struct drm_connector_state *conn_state;
• struct drm_crtc *crtc;
• struct drm_crtc_state *crtc_state;
• if (!mgr) {

• DRM_DEBUG_ATOMIC("[MST MGR:%p] Passed Topology Manager pointer
  

is pointing to NULL\n", mgr);

• return -EINVAL;
  

• }

I'd get rid of this check, we'll notice pretty easily if it's NULL :P

• mst_state = drm_atomic_get_mst_topology_state(state, mgr);

Forgot to check IS_ERR(mst_state) here

• list_for_each_entry(pos, &mst_state->vcpis, next) {

• connector = pos->port->connector;
  

• if (!connector)
  

• 	return -EINVAL;
  

• conn_state = drm_atomic_get_connector_state(state, connector);
  

• if (IS_ERR(conn_state))
  

• 	return PTR_ERR(conn_state);
  

• crtc = conn_state->crtc;
  

• if (!crtc)
  

• 	return -EINVAL;
  

This seems like something that would be an error from a driver using the API incorrectly, maybe this should be something like

if (WARN_ON(!crtc)) return -EINVAL;

• if (!drm_dp_mst_dsc_aux_for_port(pos->port))
  

• 	continue;
  

• crtc_state = drm_atomic_get_crtc_state(mst_state->base.state,
  

crtc);

• if (IS_ERR(crtc_state))
  

• 	return PTR_ERR(crtc_state);
  

• DRM_DEBUG_ATOMIC("[MST MGR:%p] Setting mode_changed flag on
  

CRTC %p\n", mgr, crtc);

This can be wrapped a bit more to fit in 80 chars

• crtc_state->mode_changed = true;
  

Nitpick here: I usually try to group assignments and conditional checks on those assignments unless it makes it more difficult to read, like:

ret = cool_function(); if (ret) return ret;

But not too big of a deal either way. Won't hold back review

• }
• return 0;

+} +EXPORT_SYMBOL(drm_dp_mst_add_affected_dsc_crtcs);

/**

• drm_dp_mst_atomic_enable_dsc - Set DSC Enable Flag to On/Off
• @state: Pointer to the new drm_atomic_state

diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h index 2919d9776af3..10e9c7049061 100644 --- a/include/drm/drm_dp_mst_helper.h +++ b/include/drm/drm_dp_mst_helper.h @@ -779,6 +779,8 @@ int drm_dp_mst_atomic_enable_dsc(struct drm_atomic_state *state, struct drm_dp_mst_port *port, int pbn, int pbn_div, bool enable); +int drm_dp_mst_add_affected_dsc_crtcs(struct drm_atomic_state *state,

• 		      struct drm_dp_mst_topology_mgr *mgr);
  

I'd add a __must_check attribute here. With the more important changes addressed here:

Reviewed-by: Lyude Paul lyude@redhat.com

int __must_check drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_mst_topology_mgr *mgr,