[PATCH v4 5/5] drm/dp-mst: Add helpers for querying and enabling MST DSC

[Lyude Paul]

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> Add drm_dp_mst_dsc_caps_for_port and drm_dp_mst_dsc_enable,
> two helper functions for MST DSC
> 
> The former, given a port, returns the raw DPCD DSC caps off
> that port.
> 
> The latter, given a port, enables or disables DSC on that port.
> 
> In both cases, the port given as input should be a leaf of
> the MST tree with an attached display.
> 
> The logic for this is somewhat complicated, as DSC can be
> enabled in 4 different ways.
> 
> Case 1: DP-to-DP peer device
> if the branch immediately upstream has
>  - PDT = DP_PEER_DEVICE_DP_MST_BRANCHING (2)
>  - DPCD rev. >= DP 1.4
>  - Exactly one input and one output
>  - The output has PDT = DP_PEER_DEVICE_SST_SINK (3)
> 
> In this case, DSC could be possible either on the endpoint
> or the peer device. Prefer the endpoint, which is possible if
>  - The endpoint has DP_DSC_DECOMPRESSION_IS_SUPPORTED bit set
>  - The endpoint has DP_FEC_CAPABLE bit set
>  - The peer device has DSC_PASSTHROUGH_CAPABILITY bit set (from DP v2.0)
> 
> Otherwise, use the peer device
> 
> Case 2: DP-to-HDMI peer device
> If the output port has
>  - PDT = DP_PEER_DEVICE_DP_LEGACY_CONV (4)
>  - DPCD rev. >= DP 1.4
>  - LDPS = true
>  - MCS = false
> 
> In this case, DSC can only be attempted on the peer device
> (the output port)
> 
> Case 3: Virtual DP Sink (Internal Display Panel)
> If the output port has
>  - DPCD rev. >= DP 1.4
>  - port_num >= 8
> 
> In this case, DSC can only be attempted on the peer device
> (the output port)
> 
> Case 4: Synaptix Workaround
> If the output has
>  - link DPCD rev. >= DP 1.4
>  - link branch_dev_id = 0x90CC24 (Synaptix)
>  - There is exactly one branch device between the link and output
> 
> In this case, DSC can be attempted, but only using the *link*
> aux device's caps. This is a quirk.
> 
> Cc: Lyude Paul <lyude at redhat.com>
> Cc: Wenjing Liu <Wenjing.Liu at amd.com>
> Cc: Nikola Cornij <Nikola.Cornij at amd.com>
> Signed-off-by: David Francis <David.Francis at amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 195 ++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |   3 +
>  2 files changed, 198 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index af4b5cec7c84..00ddc54af65b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4186,3 +4186,198 @@ static void drm_dp_mst_unregister_i2c_bus(struct
> drm_dp_aux *aux)
>  {
>  	i2c_del_adapter(&aux->ddc);
>  }
> +
> +/**
> + * drm_dp_mst_is_virtual_dpcd() - Is the given port a virtual DPCD device?
> + * @port: The port to check
> + *

Mind elaborating a bit here in the kernel documentation on what a virtual DPCD
device is?

> + * Returns:
> + * true if the port is a virtual DPCD peer device, false otherwise
> + */
> +static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
> +{
> +	struct drm_dp_mst_port *downstream_port;
> +
> +	if (!port)
> +		return false;

I'd drop this, and just move the if (!port) check into
drm_dp_mst_dsc_aux_for_port(). Makes things a little more explicit.

> +
> +	/* Virtual DP Sink (Internal Display Panel) */
> +	if (port->port_num >= 8 && port->dpcd_rev >= DP_DPCD_REV_14)
> +		return true;
> +
> +	/* DP-to-HDMI Protocol Converter */
> +	if (port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV &&
> +			!port->mcs &&
> +			port->ldps &&
> +			port->dpcd_rev >= DP_DPCD_REV_14)
> +		return true;

Please fix the indenting here, most DRM code (I may have mistakenly pointed
you at the kernel style guidelines, but I realized they don't explicitly
mention this other then the rule of "your code should look like the code
around it") written with line continuations starting on the same column as the
beginning paranthesis

if (some_very_long_variable == another_very_long_variable_wow ||
    every_identifier_here_is_long > but_what_if_we_have_a_sub_conditional ||
    (then_we_end_up_formatting_it_like_this &&
     starting_at_the_beginning_paranthesis)) {
	same_for_function_calls("Which also require very long lines\n",
	                        foo, bar, baz);
}

> +
> +	/* DP-to-DP */
> +	if (port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
> +			port->mstb &&
> +			port->dpcd_rev >= DP_DPCD_REV_14 &&
> +			port->mstb->num_ports == 2) {
> +		list_for_each_entry(downstream_port, &port->mstb->ports, next)
> {
> +			if (!downstream_port->input &&
> +				downstream_port->pdt ==
> DP_PEER_DEVICE_SST_SINK)
> +				return true;
> +		}
This isn't safe, anything that iterates downwards (so either mstb->ports, or
port->mstb) in the MST topology needs to happen under &mgr->lock, since
otherwise the port list can change under us. Note there's probably some places
this isn't followed, but I'm in the process of fixing that:

https://patchwork.freedesktop.org/patch/318683/?series=63847&rev=1
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * drm_dp_mst_is_virtual_dpcd() - Test for Synaptix DSC quirk

I think you messed up a bunch of your kerneldoc comments by accident ^
> + * @port: The port to check
> + *
> + * Some Synaptix MST hubs support DSC even though they do not support
> virtual
> + * DPCD. This is a quirk.
> + *
> + * Returns:
> + * true if the Synaptix workaround is required, false otherwise
> + */
> +static bool drm_dp_mst_dsc_synaptix_workaround(struct drm_dp_mst_port
> *port)
> +{
> +	u8 data[3] = { 0 };
> +	u32 dev_id;
> +	struct drm_dp_aux *phys_aux;
> +
> +	/* The hub must be directly connected to the connector */
> +	if (port->mgr->mst_primary != port->parent)
> +		return false;
> +
> +	phys_aux = port->mgr->aux;
> +	if (drm_dp_dpcd_read(phys_aux, DP_BRANCH_OUI, data, 3) < 0)
> +		return false;
> +	dev_id = (data[0] << 16) & (data[1] << 8) & data[3];
> +	/* Synaptix device ID */
> +	if (dev_id != 0x90CC24)
> +		return false;

More things I'm noticing now that I've got this applied locally. I originally
suggested using only the dpcd_quirks list here, but it appears I didn't read
this function closely enough and missed the fact you have to read from
multiple aux ports here. Huh.

So, first off we don't need to read the ident by hand here, we should just use
the helper in drm_dp_read_desc() and add a quirk for this into
dpcd_quirk_list[]. Maybe something like
DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD. This also ives us the benefit of
having this information printed out into the kernel debug log.

Plus, you accidentally go out of bounds when you check data[3] anyway ;)

> +
> +	if (drm_dp_dpcd_read(phys_aux, DP_DPCD_REV, data, 1) < 0)
> +		return false;
> +	/* Must be DPCD rev. 1.4 or later */
> +	if (data[0] < DP_DPCD_REV_14)
> +		return false;

No need to reread the DPCD here, just use the cached dpcd in port->mgr->edid

> +
> +	if (drm_dp_dpcd_read(&port->aux,
> +			DP_DOWNSTREAMPORT_PRESENT, data, 1) < 0)
More indenting to fix

> +		return false;
> +	/* Must not be a VGA converter */
> +	if ((data[0] & 7) == 3)
Let's just use the proper macros from include/drm/drm_dp_helper.h here

> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
> + * @port: The port to check. A leaf of the MST tree with an attached
> display.
> + *
> + * Depending on the situation, DSC may be enabled via the endpoint aux,
> + * the immediately upstream aux, or the connector's physical aux.
> + *
> + * Returns:
> + * NULL if DSC cannot be enabled on this port, otherwise the aux device
> + */
> +struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port
> *port)
> +{
> +	u8 upstream_dsc = 0;
> +	u8 endpoint_dsc = 0;
> +	u8 endpoint_fec = 0;
> +	struct drm_dp_mst_port *immediate_upstream_port;
> +	struct drm_dp_mst_port *fec_port;
> +
> +	if (port && port->parent)
> +		immediate_upstream_port = port->parent->port_parent;
> +	else
> +		immediate_upstream_port = NULL;
> +
> +	fec_port = immediate_upstream_port;
> +	while (fec_port) {
> +		if (!fec_port->fec_capable)
> +			return NULL;
> +
> +		fec_port = fec_port->parent->port_parent;
> +	}
> +
> +	if (immediate_upstream_port) {
> +		if (drm_dp_dpcd_read(&immediate_upstream_port->aux,
> +				DP_DSC_SUPPORT, &upstream_dsc, 1) < 0)
> +			return NULL;
> +	}
> +
> +	if (drm_dp_dpcd_read(&port->aux,
> +			DP_DSC_SUPPORT, &endpoint_dsc, 1) < 0)
> +		return NULL;
> +	if (drm_dp_dpcd_read(&port->aux,
> +			DP_FEC_CAPABILITY, &endpoint_fec, 1) < 0)
> +		return NULL;
> +
> +	/* Enpoint decompression with DP-to-DP peer device */
> +	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port)
> +			&& (upstream_dsc & 0x2) /* DSC passthrough */
> +			&& (endpoint_fec & DP_FEC_CAPABLE)
> +			&& (endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED))
> +		return &port->aux;
> +
> +	/* Virtual DPCD decompression with DP-to-DP peer device */
> +	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port))
> +		return &immediate_upstream_port->aux;
> +
> +	/* Virtual DPCD decompression with DP-to-HDMI or Virtual DP Sink */
> +	if (drm_dp_mst_is_virtual_dpcd(port))
> +		return &port->aux;
> +
> +	/* Synaptix workaround */
> +	if (drm_dp_mst_dsc_synaptix_workaround(port))
> +		return port->mgr->aux;
> +
> +	return NULL;
> +}
> +
> +/**
> + * drm_dp_mst_dsc_aux_for_port() - Retrieve the DSC capability registers
> + * @port: The port to check. A leaf of the MST tree with an attached
> display.
> + * @caps: Output.  A pointer to an array at least 16 bytes long
> + *
> + * Reads the DSC capability registers (DSC_SUPPORT through
> + * BITS_PER_PIXEL_INCREMENT) and store them in the given pointer. Use
> + * the correct aux for DSC on the given port.
> + *
> + * Returns:
> + * The number of bytes read on success, or a negative error code on failure
> + */
> +int drm_dp_mst_dsc_caps_for_port(struct drm_dp_mst_port *port, u8 *caps)
> +{
> +	struct drm_dp_aux *aux = drm_dp_mst_dsc_aux_for_port(port);
> +
> +	if (!aux)
> +		return -EINVAL;
> +
> +	return drm_dp_dpcd_read(aux, DP_DSC_SUPPORT, caps, 16);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_dsc_caps_for_port);
> +
> +/**
> + * drm_dp_mst_dsc_aux_for_port() - Enable DSC on an MST endpoint
> + * @port: The port to check. A leaf of the MST tree with an attached
> display.
> + * @enable: true for turn on DSC, false for turn off DSC
> + *
> + * Writes DP_DSC_ENABLE on the correct aux for the given port.
> + *
> + * Returns:
> + * The number of bytes written on success, or a negative error code on
> failure
> + */
> +int drm_dp_mst_dsc_enable(struct drm_dp_mst_port *port, bool enable)
> +{
> +	struct drm_dp_aux *aux = drm_dp_mst_dsc_aux_for_port(port);
> +	u8 enable_dsc = enable ? 1 : 0;
> +
> +	if (!aux)
> +		return -EINVAL;
> +
> +	return drm_dp_dpcd_write(aux, DP_DSC_ENABLE, &enable_dsc, 1);
> +}

Why not just make it so that drm_dp_mst_dsc_caps() also returns the
appropriate drm_dp_aux device to use, and then pass the pointer to
drm_dp_mst_dsc_enable() instead of passing it the drm_dp_mst_port*? That way
we won't have to probe for these caps twice.

> +EXPORT_SYMBOL(drm_dp_mst_dsc_enable);
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index fa973773a4a7..0f70dc8dfbeb 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -674,6 +674,9 @@ int __must_check drm_dp_mst_atomic_check(struct
> drm_atomic_state *state);
>  void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
>  void drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port);
>  
> +int drm_dp_mst_dsc_caps_for_port(struct drm_dp_mst_port *port, u8 *caps);
> +int drm_dp_mst_dsc_enable(struct drm_dp_mst_port *port, bool enable);
> +
>  extern const struct drm_private_state_funcs
> drm_dp_mst_topology_state_funcs;
>  
>  /**
-- 
Cheers,
	Lyude Paul