[12/13] drm/dp_mst: Add DSC enablement helpers to DRM
diff mbox series

Message ID 20191030192431.5798-13-mikita.lipski@amd.com
State New
Headers show
Series
  • DSC MST support for AMDGPU
Related show

Commit Message

Lipski, Mikita Oct. 30, 2019, 7:24 p.m. UTC
From: Mikita Lipski <mikita.lipski@amd.com>

Adding the following elements to add MST DSC support to DRM:

- dsc_enable boolean flag to drm_dp_vcpi_allocation structure to signal,
which port got DSC enabled

- function drm_dp_helper_update_vcpi_slots_for_dsc allows reallocation
of newly recalculated VCPI slots and raises dsc_enable flag on the port.

- function drm_dp_mst_update_dsc_crtcs is called in drm_dp_mst_atomic_check,
its purpose is to iterate through all the ports in the topology and set
mode_changed flag on crtc if DSC has been enabled.

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 103 +++++++++++++++++++++++++-
 include/drm/drm_dp_mst_helper.h       |   4 +
 2 files changed, 106 insertions(+), 1 deletion(-)

Comments

Lyude Paul Nov. 5, 2019, 12:31 a.m. UTC | #1
Hey! Great start so far, some comments down below:

On Wed, 2019-10-30 at 15:24 -0400, mikita.lipski@amd.com wrote:
> From: Mikita Lipski <mikita.lipski@amd.com>
> 
> Adding the following elements to add MST DSC support to DRM:
> 
> - dsc_enable boolean flag to drm_dp_vcpi_allocation structure to signal,
> which port got DSC enabled
> 
> - function drm_dp_helper_update_vcpi_slots_for_dsc allows reallocation
> of newly recalculated VCPI slots and raises dsc_enable flag on the port.
> 
> - function drm_dp_mst_update_dsc_crtcs is called in drm_dp_mst_atomic_check,
> its purpose is to iterate through all the ports in the topology and set
> mode_changed flag on crtc if DSC has been enabled.
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 103 +++++++++++++++++++++++++-
>  include/drm/drm_dp_mst_helper.h       |   4 +
>  2 files changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d5df02315e14..4f2f09fe32f8 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -73,6 +73,7 @@ static bool drm_dp_validate_guid(struct
> drm_dp_mst_topology_mgr *mgr,
>  static int drm_dp_mst_register_i2c_bus(struct drm_dp_aux *aux);
>  static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux);
>  static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr);
> +static void drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state
> *mst_state);
>  
>  #define DP_STR(x) [DP_ ## x] = #x
>  
> @@ -3293,6 +3294,65 @@ int drm_dp_atomic_find_vcpi_slots(struct
> drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
>  
> +/**
> + * drm_dp_helper_update_vcpi_slots_for_dsc() - Update VCPI slots with new
> on the state
> + *
> + * @state: global atomic state
> + * @port: port to find vcpi slots
> + * @pbn: updated bandwidth required for the mode in PBN
> + *
> + * Function reallocates VCPI slots to the @port by calling
> + * drm_dp_atomic_find_vcpi_slots. The assumption is that VCPI slots
> + * have already been allocated and this is second call overwritting
> + * initial values. After the VCPI is allocated dsc_enable flag is set to
> + * true for atomic check.
> + *
> + * It is driver's responsibility to call this function after it decides
> + * to enable DSC.
> + *
> + * See also:
> + * drm_dp_mst_update_dsc_crtcs()
> + *
> + * Returns:
> + * Total slots in the atomic state assigned for this port, or a negative
> error
> + * code if the port no longer exists or vcpi slots haven't been assigned.
> + */
> +int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state,
> +					    struct drm_dp_mst_port *port,
> +					    int pbn)
> +{
> +	struct drm_dp_mst_topology_state *topology_state;
> +	struct drm_dp_vcpi_allocation *pos;
> +	bool found = false;
> +	int vcpi = 0;
> +
> +	topology_state = drm_atomic_get_mst_topology_state(state, port->mgr);
> +
> +	if (IS_ERR(topology_state))
> +		return PTR_ERR(topology_state);
> +
> +	list_for_each_entry(pos, &topology_state->vcpis, next) {
> +		if (pos->port == port) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!found || !pos->vcpi)
> +		return -EINVAL;
> +
> +	vcpi = drm_dp_atomic_find_vcpi_slots(state, port->mgr,
> +					     port, pbn);
> +
> +	if (vcpi < 0)
> +		return -EINVAL;
> +
> +	pos->dsc_enable = true;
> +
> +	return vcpi;
> +}
> +
This helper I think we can simplify a bit by dropping it and merging it with
drm_dp_mst_update_dsc_crtcs(). I've got a more in-depth explanation of what I
mean down below:

> +EXPORT_SYMBOL(drm_dp_helper_update_vcpi_slots_for_dsc);
>  /**
>   * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
>   * @state: global atomic state
> @@ -3871,6 +3931,46 @@ drm_dp_mst_atomic_check_topology_state(struct
> drm_dp_mst_topology_mgr *mgr,
>  	return 0;
>  }
>  
> +/**
> + * drm_dp_mst_update_dsc_crtcs - Set mode change flag on CRTCs which
> + * just got DSC enabled
> + * @state: Pointer to the new &struct drm_dp_mst_topology_state
> + *
> + * Itearate through all the ports in MST topology to check if DSC
> + * has been enabled on any of them. Set mode_changed to true on
> + * crtc state that just got DSC enabled.
> + *
> + * See also:
> + * drm_dp_helper_update_vcpi_slots_for_dsc()
> + */
> +static void
> +drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state *mst_state)
> +{

Just grab the atomic state from within this function, not really much point in
making the caller pull in the mst topoloy state since we're the only ones
using it

> +	struct drm_dp_vcpi_allocation *pos;
> +	struct drm_dp_mst_port *port;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +
> +	list_for_each_entry(pos, &mst_state->vcpis, next) {
> +
> +		port = pos->port;
> +		conn_state = drm_atomic_get_connector_state(mst_state-
> >base.state,
> +							    port->connector);
> +		crtc = conn_state->crtc;
> +		if (!crtc)
> +			continue;
> +
> +		crtc_state = drm_atomic_get_crtc_state(mst_state->base.state,
> crtc);

You're forgetting to check the return status of drm_atomic_get_crtc_state(),
since it can fail here. You need something like:

if (IS_ERR(crtc_state))
	return PTR_ERR(crtc_state);


> +		if (port->vcpi.vcpi == pos->vcpi)
> +			continue;
> +
> +		if (pos->dsc_enable) {
> +			crtc_state->mode_changed = true;
> +			pos->dsc_enable = false;
> +		}
> +	}
> +}

So: I think we can simply this a bit. Why not rename
drm_dp_mst_update_dsc_crtcs() to drm_dp_mst_atomic_enable_dsc(), and rework it
to look like this psuedocode:

int drm_dp_mst_atomic_set_dsc(struct drm_atomic_state *state,
                              struct drm_dp_mst_port *port,
                              bool enabled)
{
	struct drm_dp_mst_topology_state *mst_state = /* ... */;
	/* ... */
	if (port->dsc_enable == enabled)
		return 0;

	port->dsc_enable = enabled;
	for_each_mst_port() {
		if (port->dsc_enable == enabled)
			continue;
		port->dsc_enable = enabled;

		conn_state = drm_atomic_get_connector_state(port->connector);
		/* error handling ... */
		if (!conn_state->crtc)
			continue;
		
		crtc_state = drm_atomic_get_crtc_state(conn_state->crtc);
		/* error handling... */
		crtc_state->mode_changed = true;
	}

	return 0;
}

IMO this works a bit better since all we really need is something to pull in
crtcs affected by the state change.

Also, more DRM_DEBUG_ATOMIC() statements similar to what I've got in the vcpi
helpers would be nice :)

Otherwise, looks great so far!
>  /**
>   * drm_dp_mst_atomic_check - Check that the new state of an MST topology in
> an
>   * atomic update is valid
> @@ -3887,9 +3987,9 @@ drm_dp_mst_atomic_check_topology_state(struct
> drm_dp_mst_topology_mgr *mgr,
>   * See also:
>   * drm_dp_atomic_find_vcpi_slots()
>   * drm_dp_atomic_release_vcpi_slots()
> - *
>   * Returns:
>   *
> + *
>   * 0 if the new state is valid, negative error code otherwise.
>   */
>  int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
> @@ -3902,6 +4002,7 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state
> *state)
>  		ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
>  		if (ret)
>  			break;
> +		drm_dp_mst_update_dsc_crtcs(mst_state);
>  	}
>  
>  	return ret;
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index 4cf738545dfb..185e29895f5f 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -431,6 +431,7 @@ struct drm_dp_payload {
>  struct drm_dp_vcpi_allocation {
>  	struct drm_dp_mst_port *port;
>  	int vcpi;
> +	bool dsc_enable;
>  	struct list_head next;
>  };
>  
> @@ -662,6 +663,9 @@ int __must_check
>  drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
>  			      struct drm_dp_mst_topology_mgr *mgr,
>  			      struct drm_dp_mst_port *port, int pbn);
> +int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state,
> +					    struct drm_dp_mst_port *port,
> +					    int pbn);
>  int __must_check
>  drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
>  				 struct drm_dp_mst_topology_mgr *mgr,

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index d5df02315e14..4f2f09fe32f8 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -73,6 +73,7 @@  static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
 static int drm_dp_mst_register_i2c_bus(struct drm_dp_aux *aux);
 static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux);
 static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr);
+static void drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state *mst_state);
 
 #define DP_STR(x) [DP_ ## x] = #x
 
@@ -3293,6 +3294,65 @@  int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
 
+/**
+ * drm_dp_helper_update_vcpi_slots_for_dsc() - Update VCPI slots with new on the state
+ *
+ * @state: global atomic state
+ * @port: port to find vcpi slots
+ * @pbn: updated bandwidth required for the mode in PBN
+ *
+ * Function reallocates VCPI slots to the @port by calling
+ * drm_dp_atomic_find_vcpi_slots. The assumption is that VCPI slots
+ * have already been allocated and this is second call overwritting
+ * initial values. After the VCPI is allocated dsc_enable flag is set to
+ * true for atomic check.
+ *
+ * It is driver's responsibility to call this function after it decides
+ * to enable DSC.
+ *
+ * See also:
+ * drm_dp_mst_update_dsc_crtcs()
+ *
+ * Returns:
+ * Total slots in the atomic state assigned for this port, or a negative error
+ * code if the port no longer exists or vcpi slots haven't been assigned.
+ */
+int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state,
+					    struct drm_dp_mst_port *port,
+					    int pbn)
+{
+	struct drm_dp_mst_topology_state *topology_state;
+	struct drm_dp_vcpi_allocation *pos;
+	bool found = false;
+	int vcpi = 0;
+
+	topology_state = drm_atomic_get_mst_topology_state(state, port->mgr);
+
+	if (IS_ERR(topology_state))
+		return PTR_ERR(topology_state);
+
+	list_for_each_entry(pos, &topology_state->vcpis, next) {
+		if (pos->port == port) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found || !pos->vcpi)
+		return -EINVAL;
+
+	vcpi = drm_dp_atomic_find_vcpi_slots(state, port->mgr,
+					     port, pbn);
+
+	if (vcpi < 0)
+		return -EINVAL;
+
+	pos->dsc_enable = true;
+
+	return vcpi;
+}
+
+EXPORT_SYMBOL(drm_dp_helper_update_vcpi_slots_for_dsc);
 /**
  * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
  * @state: global atomic state
@@ -3871,6 +3931,46 @@  drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,
 	return 0;
 }
 
+/**
+ * drm_dp_mst_update_dsc_crtcs - Set mode change flag on CRTCs which
+ * just got DSC enabled
+ * @state: Pointer to the new &struct drm_dp_mst_topology_state
+ *
+ * Itearate through all the ports in MST topology to check if DSC
+ * has been enabled on any of them. Set mode_changed to true on
+ * crtc state that just got DSC enabled.
+ *
+ * See also:
+ * drm_dp_helper_update_vcpi_slots_for_dsc()
+ */
+static void
+drm_dp_mst_update_dsc_crtcs(struct drm_dp_mst_topology_state *mst_state)
+{
+	struct drm_dp_vcpi_allocation *pos;
+	struct drm_dp_mst_port *port;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+
+	list_for_each_entry(pos, &mst_state->vcpis, next) {
+
+		port = pos->port;
+		conn_state = drm_atomic_get_connector_state(mst_state->base.state,
+							    port->connector);
+		crtc = conn_state->crtc;
+		if (!crtc)
+			continue;
+
+		crtc_state = drm_atomic_get_crtc_state(mst_state->base.state, crtc);
+		if (port->vcpi.vcpi == pos->vcpi)
+			continue;
+
+		if (pos->dsc_enable) {
+			crtc_state->mode_changed = true;
+			pos->dsc_enable = false;
+		}
+	}
+}
 /**
  * drm_dp_mst_atomic_check - Check that the new state of an MST topology in an
  * atomic update is valid
@@ -3887,9 +3987,9 @@  drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,
  * See also:
  * drm_dp_atomic_find_vcpi_slots()
  * drm_dp_atomic_release_vcpi_slots()
- *
  * Returns:
  *
+ *
  * 0 if the new state is valid, negative error code otherwise.
  */
 int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
@@ -3902,6 +4002,7 @@  int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
 		ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
 		if (ret)
 			break;
+		drm_dp_mst_update_dsc_crtcs(mst_state);
 	}
 
 	return ret;
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 4cf738545dfb..185e29895f5f 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -431,6 +431,7 @@  struct drm_dp_payload {
 struct drm_dp_vcpi_allocation {
 	struct drm_dp_mst_port *port;
 	int vcpi;
+	bool dsc_enable;
 	struct list_head next;
 };
 
@@ -662,6 +663,9 @@  int __must_check
 drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 			      struct drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_mst_port *port, int pbn);
+int drm_dp_helper_update_vcpi_slots_for_dsc(struct drm_atomic_state *state,
+					    struct drm_dp_mst_port *port,
+					    int pbn);
 int __must_check
 drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
 				 struct drm_dp_mst_topology_mgr *mgr,