RE: [PATCH v2 11/14] drm/amd/display: Validate DSC caps on MST endpoints

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Francis and Nicholas,

I have an internal document before explaining all this in more detail.
I have attached it here.
Can you check if this is aligned with your implementation.
Note that we don't support YCbCr 4:2:2 Native now.

Thanks,
Wenjing

-----Original Message-----
From: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx> 
Sent: Wednesday, August 21, 2019 2:15 PM
To: Francis, David <David.Francis@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Liu, Wenjing <Wenjing.Liu@xxxxxxx>; Cornij, Nikola <Nikola.Cornij@xxxxxxx>
Subject: Re: [PATCH v2 11/14] drm/amd/display: Validate DSC caps on MST endpoints

On 8/20/19 3:12 PM, David Francis wrote:
> The first step in MST DSC is checking MST endpoints to see how DSC can 
> be enabled
> 
> 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.
> 
> Test for these cases as modes are enumerated for an MST endpoint.
> We cannot do this during link attach because the dc_sink object will 
> not have been created yet
> 
> If no DSC is attempted, zero the DSC caps
> 
> Cc: Wenjing Liu <Wenjing.Liu@xxxxxxx>
> Cc: Nikola Cornij <Nikola.Cornij@xxxxxxx>
> Signed-off-by: David Francis <David.Francis@xxxxxxx>

Questions and comments below...

> ---
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 123 +++++++++++++++++-
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.h   |   3 +
>   2 files changed, 125 insertions(+), 1 deletion(-)
> 
> diff --git 
> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 16218a202b59..58571844f6d5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -25,6 +25,7 @@
>   
>   #include <linux/version.h>
>   #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>   #include "dm_services.h"
>   #include "amdgpu.h"
>   #include "amdgpu_dm.h"
> @@ -189,6 +190,120 @@ static const struct drm_connector_funcs dm_dp_mst_connector_funcs = {
>   	.early_unregister = amdgpu_dm_mst_connector_early_unregister,
>   };
>   
> +bool is_virtual_dpcd(struct drm_dp_mst_port *port) {
> +	struct drm_dp_mst_port *downstream_port;
> +
> +	if (!port)
> +		return false;
> +
> +	if (port->port_num >= 8 &&
> +		port->dpcd_rev >= DP_DPCD_REV_14) {

All these if statements should be aligned if possible. That's just formatting nitpicks though.

> +		/* Virtual DP Sink (Internal Display Panel) */
> +		return true;
> +	} else if (port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV &&
> +			!port->mcs &&
> +			port->ldps &&
> +			port->dpcd_rev >= DP_DPCD_REV_14) {
> +		/* DP-to-HDMI Protocol Converter */
> +		return true;
> +	} else if (port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
> +			port->mstb &&
> +			port->dpcd_rev >= DP_DPCD_REV_14) {
> +		/* DP-to-DP */
> +		if (port->mstb->num_ports == 2) {

Can probably merge this if condition into the else if above. Will help cut down on line length below.

> +			list_for_each_entry(downstream_port, &port->mstb->ports, next) {
> +				if (!downstream_port->input &&
> +						downstream_port->pdt == DP_PEER_DEVICE_SST_SINK)
> +					return true;
> +			}
> +		}
> +	}
> +	return false;
> +} > +
> +bool synaptix_workaround(struct amdgpu_dm_connector *aconnector)

This probably needs a better name. This isn't applying a workaround for a specific device but returning true if it is a specific device.

> +{
> +	struct drm_dp_mst_port *port = aconnector->port;
> +	struct dc_link *link = aconnector->dc_sink->link;
> +	u8 down_stream_port_data;
> +
> +	if (port->mgr->mst_primary == port->parent &&
> +			link->dpcd_caps.branch_dev_id == 0x90CC24 &&
> +			link->dpcd_caps.dpcd_rev.raw >= DP_DPCD_REV_14) {
> +		drm_dp_mst_dpcd_read(&port->aux, DP_DOWNSTREAMPORT_PRESENT, &down_stream_port_data, 1);
> +		if ((down_stream_port_data & 7) != 3)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> +static bool validate_dsc_caps_on_connector(struct amdgpu_dm_connector 
> +*aconnector) {
> +	u8 upstream_dsc_caps[16] = { 0 };
> +	u8 endpoint_dsc_caps[16] = { 0 };
> +	u8 endpoint_fec_caps = 0;
> +	struct dc_sink *dc_sink = aconnector->dc_sink;
> +	struct drm_dp_mst_port *output_port = aconnector->port;
> +	struct drm_dp_mst_port *immediate_upstream_port;
> +	struct drm_dp_mst_port *fec_port;
> +
> +	if (aconnector->port && aconnector->port->parent)
> +		immediate_upstream_port = aconnector->port->parent->port_parent;
> +	else
> +		immediate_upstream_port = NULL;
> +
> +	fec_port = immediate_upstream_port;
> +	while (fec_port) {
> +		if (!fec_port->fec_capable)
> +			return false;
> +
> +		fec_port = fec_port->parent->port_parent;
> +	}
> +
> +	if (immediate_upstream_port)
> +		drm_dp_mst_dpcd_read(&immediate_upstream_port->aux, DP_DSC_SUPPORT, upstream_dsc_caps, 16);
> +	drm_dp_mst_dpcd_read(&output_port->aux, DP_DSC_SUPPORT, endpoint_dsc_caps, 16);
> +	drm_dp_mst_dpcd_read(&output_port->aux, DP_FEC_CAPABILITY, 
> +&endpoint_fec_caps, 1);
> +
> +	if (is_virtual_dpcd(immediate_upstream_port)
> +			&& (upstream_dsc_caps[0] & 0x2) /* DSC passthrough capability */
> +			&& (endpoint_fec_caps & DP_FEC_CAPABLE)
> +			&& (endpoint_dsc_caps[0] & DP_DSC_DECOMPRESSION_IS_SUPPORTED)) {
> +		/* Enpoint decompression with DP-to-DP peer device */
> +		if (!dc_dsc_parse_dsc_dpcd(endpoint_dsc_caps, NULL, &dc_sink->sink_dsc_caps.dsc_dec_caps))
> +			return false;
> +
> +		dc_sink->sink_dsc_caps.is_virtual_dpcd_dsc = false;
> +	} else if (is_virtual_dpcd(immediate_upstream_port)) {
> +		/* Virtual DPCD decompression with DP-to-DP peer device */
> +		if (!dc_dsc_parse_dsc_dpcd(upstream_dsc_caps, NULL, &dc_sink->sink_dsc_caps.dsc_dec_caps))
> +			return false;
> +
> +		dc_sink->sink_dsc_caps.is_virtual_dpcd_dsc = true;
> +	} else if (is_virtual_dpcd(output_port)) {
> +		/* Virtual DPCD decompression with DP-to-HDMI or Virtual DP Sink */
> +		if (!dc_dsc_parse_dsc_dpcd(endpoint_dsc_caps, NULL, &dc_sink->sink_dsc_caps.dsc_dec_caps))
> +			return false;
> +
> +		dc_sink->sink_dsc_caps.is_virtual_dpcd_dsc = true;
> +	} else if (synaptix_workaround(aconnector)) {
> +		/* Synaptix workaround */
> +		aconnector = dc_sink->link->priv;
> +		drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, DP_DSC_SUPPORT, upstream_dsc_caps, 16);
> +		if (!dc_dsc_parse_dsc_dpcd(upstream_dsc_caps, NULL, &dc_sink->sink_dsc_caps.dsc_dec_caps))
> +			return false;
> +
> +		dc_sink->sink_dsc_caps.is_virtual_dpcd_dsc = true;
> +	} else {
> +		return false;
> +	}
> +
> +	return true;
> +}
> +#endif

Should this be common code instead as a dp mst helper for determining caps? Other than setting the dc sink stuff the rest looks like it could be common but I'm not overly familiar with the architecture.

> +
>   static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   {
>   	struct amdgpu_dm_connector *aconnector = 
> to_amdgpu_dm_connector(connector);
> @@ -231,10 +346,16 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   		/* dc_link_add_remote_sink returns a new reference */
>   		aconnector->dc_sink = dc_sink;
>   
> -		if (aconnector->dc_sink)
> +		if (aconnector->dc_sink) {
>   			amdgpu_dm_update_freesync_caps(
>   					connector, aconnector->edid);
>   
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> +			if (!validate_dsc_caps_on_connector(aconnector))
> +				memset(&aconnector->dc_sink->sink_dsc_caps,
> +				       0, sizeof(aconnector->dc_sink->sink_dsc_caps));
> +#endif
> +		}
>   	}
>   
>   	drm_connector_update_edid_property(
> diff --git 
> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> index 2da851b40042..8de3d8c30f8d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.h
> @@ -32,4 +32,7 @@ struct amdgpu_dm_connector;
>   void amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm,
>   				       struct amdgpu_dm_connector *aconnector);
>   
> +bool is_virtual_dpcd(struct drm_dp_mst_port *port); bool 
> +synaptix_workaround(struct amdgpu_dm_connector *aconnector);

These shouldn't be defined in the header if they're only going to be used here, they should just be static.

> +
>   #endif
> 

Nicholas Kazlauskas

Attachment: DSC REQUIREMENTS AND POLICIES-internal.pptx
Description: DSC REQUIREMENTS AND POLICIES-internal.pptx

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

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux