Re: [PATCH V5] drm: Add support for DP 1.4 Compliance edid corruption test

[Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6821 bytes --]

Hi Jerry,

First of all, thanks for your patch. You can see some comments inline,
just simple things.

On 01/31, Jerry (Fangzhi) Zuo wrote:
> Unlike DP 1.2 edid corruption test, DP 1.4 requires to calculate
> real CRC value of the last edid data block, and write it back.
> Current edid CRC calculates routine adds the last CRC byte,
> and check if non-zero.
> 
> This behavior is not accurate; actually, we need to return
> the actual CRC value when corruption is detected.
> This commit changes this issue by returning the calculated CRC,
> and initiate the required sequence.
> 
> Change since v5
> - Obtain real CRC value before dumping bad edid
> 
> Change since v4
> - Fix for CI.CHECKPATCH
> 
> Change since v3
> - Fix a minor typo.
> 
> Change since v2
> - Rewrite checksum computation routine to avoid duplicated code.
> - Rename to avoid confusion.
> 
> Change since v1
> - Have separate routine for returning real CRC.
> 
> Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 35 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_edid.c      | 23 ++++++++++++++++++----
>  include/drm/drm_connector.h     |  6 ++++++
>  include/drm/drm_dp_helper.h     |  3 +++
>  4 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index f629fc5494a4..18b285fa1a42 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -351,6 +351,41 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
>  
> +/**
> + * drm_dp_send_real_edid_checksum() - send back real edid checksum value
> + * @aux: DisplayPort AUX channel
> + * @real_edid_checksum: real edid checksum for the last block
> + *
> + * Returns true on success

I think this should be:

Returns:
True on success...

> + */
> +bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
> +                                    u8 real_edid_checksum)

Use tabs intead of space

> +{
> +	u8 link_edid_read = 0, auto_test_req = 0, test_resp = 0;
> +
> +	drm_dp_dpcd_read(aux, DP_DEVICE_SERVICE_IRQ_VECTOR, &auto_test_req, 1);

drm_dp_dpcd_read() documentation says:

 [..]
 Returns the number of bytes transferred on success, or a negative error
 code on failure. [..][1]

How about catching the return value of drm_dp_dpcd_read() and handle it?

1. drivers/gpu/drm/drm_dp_helper.c

> +	auto_test_req &= DP_AUTOMATED_TEST_REQUEST;
> +
> +	drm_dp_dpcd_read(aux, DP_TEST_REQUEST, &link_edid_read, 1);

Same

> +	link_edid_read &= DP_TEST_LINK_EDID_READ;
> +
> +	if (!auto_test_req || !link_edid_read) {
> +		DRM_DEBUG_KMS("Source DUT does not support TEST_EDID_READ\n");

I'm not 100% sure, but I think that drm_dbg_kms() represents the new
approach for handling debug messages. Could you confirm that? If so,
could you update it?

> +		return false;
> +	}
> +
> +	drm_dp_dpcd_write(aux, DP_DEVICE_SERVICE_IRQ_VECTOR, &auto_test_req, 1);
> +
> +	/* send back checksum for the last edid extension block data */
> +	drm_dp_dpcd_write(aux, DP_TEST_EDID_CHECKSUM, &real_edid_checksum, 1);

Again, how about handling the return from drm_dp_dpcd_write?

> +
> +	test_resp |= DP_TEST_EDID_CHECKSUM_WRITE;
> +	drm_dp_dpcd_write(aux, DP_TEST_RESPONSE, &test_resp, 1);
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_dp_send_real_edid_checksum);
> +
>  /**
>   * drm_dp_downstream_max_clock() - extract branch device max
>   *                                 pixel rate for legacy VGA
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 99769d6c9f84..f064e75fb4c5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1590,11 +1590,22 @@ static int validate_displayid(u8 *displayid, int length, int idx);
>  static int drm_edid_block_checksum(const u8 *raw_edid)
>  {
>  	int i;
> -	u8 csum = 0;
> -	for (i = 0; i < EDID_LENGTH; i++)
> +	u8 csum = 0, crc = 0;
> +
> +	for (i = 0; i < EDID_LENGTH - 1; i++)
>  		csum += raw_edid[i];
>  
> -	return csum;
> +	crc = 0x100 - csum;
> +
> +	return crc;
> +}
> +
> +static bool drm_edid_block_checksum_diff(const u8 *raw_edid, u8 real_checksum)
> +{
> +	if (raw_edid[EDID_LENGTH - 1] != real_checksum)
> +		return true;
> +	else
> +		return false;
>  }
>  
>  static bool drm_edid_is_zero(const u8 *in_edid, int length)
> @@ -1652,7 +1663,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
>  	}
>  
>  	csum = drm_edid_block_checksum(raw_edid);
> -	if (csum) {
> +	if (drm_edid_block_checksum_diff(raw_edid, csum)) {
>  		if (edid_corrupt)
>  			*edid_corrupt = true;
>  
> @@ -1793,6 +1804,10 @@ static void connector_bad_edid(struct drm_connector *connector,
>  			       u8 *edid, int num_blocks)
>  {
>  	int i;
> +	u8 num_of_ext = edid[0x7e];
> +
> +	/* Calculate real checksum for the last edid extension block data */
> +	connector->real_edid_checksum = drm_edid_block_checksum(edid + num_of_ext * EDID_LENGTH);
>  
>  	if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
>  		return;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 2113500b4075..b3815371c271 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1357,6 +1357,12 @@ struct drm_connector {
>  	 * rev1.1 4.2.2.6
>  	 */
>  	bool edid_corrupt;
> +	/**
> +	 * @real_edid_checksum: real edid checksum for corrupted edid block.
> +	 * Required in Displayport 1.4 compliance testing
> +	 * rev1.1 4.2.2.6
> +	 */
> +	u8 real_edid_checksum;
>  
>  	/** @debugfs_entry: debugfs directory for this connector */
>  	struct dentry *debugfs_entry;
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 127d6e1d3338..957a3d00ee05 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1459,6 +1459,9 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux,
>  int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  				 u8 status[DP_LINK_STATUS_SIZE]);
>  
> +bool drm_dp_send_real_edid_checksum(struct drm_dp_aux *aux,
> +				    u8 real_edid_checksum);
> +
>  int drm_dp_downstream_max_clock(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
>  				const u8 port_cap[4]);
>  int drm_dp_downstream_max_bpc(const u8 dpcd[DP_RECEIVER_CAP_SIZE],
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Rodrigo Siqueira
Software Engineer, Advanced Micro Devices (AMD)
https://siqueira.tech

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel