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

[Lyude Paul <lyude@redhat.com>]

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,
-- 
Cheers,
	Lyude Paul

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx