[PATCH] drm/amd/display: fix return value of dm_dp_aux_transfer() (V2)

[PATCH] drm/amd/display: fix return value of dm_dp_aux_transfer() (V2)

[Shirish S]

Currently the dm_dp_aux_transfer() does not parse
the return value of dal_ddc_service_read_dpcd_data(), which also
has a failure case.
This patch captures the same and ensures the i2c operation status is
sent appropriately to the drm framework.

V2: Updated commit message.

Signed-off-by: Shirish S <shirish.s@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 +++++-
 1 file changed, 5 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 782491e..7ac124d 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
@@ -115,7 +115,11 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 				msg->address,
 				msg->buffer,
 				msg->size);
-		return read_bytes;
+		if (read_bytes != msg->size &&
+		    read_bytes >= DDC_RESULT_FAILED_OPERATION)
+			return -EIO;
+		else
+			return read_bytes;
 	case DP_AUX_I2C_WRITE:
 		res = dal_ddc_service_write_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
-- 
2.7.4

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

Re: [PATCH] drm/amd/display: fix return value of dm_dp_aux_transfer() (V2)

[Harry Wentland]

On 2018-04-17 10:56 PM, Shirish S wrote:
> Currently the dm_dp_aux_transfer() does not parse
> the return value of dal_ddc_service_read_dpcd_data(), which also
> has a failure case.
> This patch captures the same and ensures the i2c operation status is
> sent appropriately to the drm framework.
> 
> V2: Updated commit message.
> 
> Signed-off-by: Shirish S <shirish.s@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 +++++-
>  1 file changed, 5 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 782491e..7ac124d 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
> @@ -115,7 +115,11 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
>  				msg->address,
>  				msg->buffer,
>  				msg->size);
> -		return read_bytes;
> +		if (read_bytes != msg->size &&
> +		    read_bytes >= DDC_RESULT_FAILED_OPERATION)

This doesn't look right. We shouldn't be returning the size or error code from the same function. This will not work if we submit a read 7 or 8 bytes and get an error.

Harry

> +			return -EIO;
> +		else
> +			return read_bytes;
>  	case DP_AUX_I2C_WRITE:
>  		res = dal_ddc_service_write_dpcd_data(
>  				TO_DM_AUX(aux)->ddc_service,
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: fix return value of dm_dp_aux_transfer() (V2)

[S, Shirish]

On 4/20/2018 11:52 PM, Harry Wentland wrote:
> On 2018-04-17 10:56 PM, Shirish S wrote:
>> Currently the dm_dp_aux_transfer() does not parse
>> the return value of dal_ddc_service_read_dpcd_data(), which also
>> has a failure case.
>> This patch captures the same and ensures the i2c operation status is
>> sent appropriately to the drm framework.
>>
>> V2: Updated commit message.
>>
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 +++++-
>>   1 file changed, 5 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 782491e..7ac124d 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
>> @@ -115,7 +115,11 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
>>   				msg->address,
>>   				msg->buffer,
>>   				msg->size);
>> -		return read_bytes;
>> +		if (read_bytes != msg->size &&
>> +		    read_bytes >= DDC_RESULT_FAILED_OPERATION)
> This doesn't look right. We shouldn't be returning the size or error code from the same function. This will not work if we submit a read 7 or 8 bytes and get an error.
Agree,  but hope you understood the issue, to re-iterate, in case of 
failure we reuturn a +ve number(7 or 8) back from dm_dp_aux_transfer() 
leading to edid read failures later.
I have 2 suggestions:
1. change the "enum ddc_result {" to #defines of -ve error codes? or
2. make enum start with 129 (> than the max read_bytes possible)?
let me know which one would be better?
Thanks,
Regards,
Shirish S
>
> Harry
>
>> +			return -EIO;
>> +		else
>> +			return read_bytes;
>>   	case DP_AUX_I2C_WRITE:
>>   		res = dal_ddc_service_write_dpcd_data(
>>   				TO_DM_AUX(aux)->ddc_service,
>>

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

Re: [PATCH] drm/amd/display: fix return value of dm_dp_aux_transfer() (V2)

[S, Shirish]

On 4/23/2018 9:53 AM, S, Shirish wrote:
>
>
> On 4/20/2018 11:52 PM, Harry Wentland wrote:
>> On 2018-04-17 10:56 PM, Shirish S wrote:
>>> Currently the dm_dp_aux_transfer() does not parse
>>> the return value of dal_ddc_service_read_dpcd_data(), which also
>>> has a failure case.
>>> This patch captures the same and ensures the i2c operation status is
>>> sent appropriately to the drm framework.
>>>
>>> V2: Updated commit message.
>>>
>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 
>>> +++++-
>>>   1 file changed, 5 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 782491e..7ac124d 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
>>> @@ -115,7 +115,11 @@ static ssize_t dm_dp_aux_transfer(struct 
>>> drm_dp_aux *aux,
>>>                   msg->address,
>>>                   msg->buffer,
>>>                   msg->size);
>>> -        return read_bytes;
>>> +        if (read_bytes != msg->size &&
>>> +            read_bytes >= DDC_RESULT_FAILED_OPERATION)
>> This doesn't look right. We shouldn't be returning the size or error 
>> code from the same function. This will not work if we submit a read 7 
>> or 8 bytes and get an error.
> Agree,  but hope you understood the issue, to re-iterate, in case of 
> failure we reuturn a +ve number(7 or 8) back from dm_dp_aux_transfer() 
> leading to edid read failures later.
> I have 2 suggestions:
> 1. change the "enum ddc_result {" to #defines of -ve error codes? or
> 2. make enum start with 129 (> than the max read_bytes possible)?
> let me know which one would be better?
Any suggestions?
> Thanks,
> Regards,
> Shirish S
>>
>> Harry
>>
>>> +            return -EIO;
>>> +        else
>>> +            return read_bytes;
>>>       case DP_AUX_I2C_WRITE:
>>>           res = dal_ddc_service_write_dpcd_data(
>>>                   TO_DM_AUX(aux)->ddc_service,
>>>
>

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

[PATCH] drm/amd/display: Don't return ddc result and read_bytes in same return value

[Harry Wentland]

The two ranges overlap.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---

Thinking of something like this if this works for you.

Harry

 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 20 +++++++++++--------
 .../gpu/drm/amd/display/dc/core/dc_link_ddc.c | 10 +++++++---
 .../gpu/drm/amd/display/dc/inc/dc_link_ddc.h  |  5 +++--
 3 files changed, 22 insertions(+), 13 deletions(-)

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 c3f3028253c3..b8dd7496b7bc 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
@@ -83,21 +83,22 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 	enum i2c_mot_mode mot = (msg->request & DP_AUX_I2C_MOT) ?
 		I2C_MOT_TRUE : I2C_MOT_FALSE;
 	enum ddc_result res;
-	ssize_t read_bytes;
+	uint32_t read_bytes = msg->size;
 
 	if (WARN_ON(msg->size > 16))
 		return -E2BIG;
 
 	switch (msg->request & ~DP_AUX_I2C_MOT) {
 	case DP_AUX_NATIVE_READ:
-		read_bytes = dal_ddc_service_read_dpcd_data(
+		res = dal_ddc_service_read_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
 				false,
 				I2C_MOT_UNDEF,
 				msg->address,
 				msg->buffer,
-				msg->size);
-		return read_bytes;
+				msg->size,
+				&read_bytes);
+		break;
 	case DP_AUX_NATIVE_WRITE:
 		res = dal_ddc_service_write_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
@@ -108,14 +109,15 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 				msg->size);
 		break;
 	case DP_AUX_I2C_READ:
-		read_bytes = dal_ddc_service_read_dpcd_data(
+		res = dal_ddc_service_read_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
 				true,
 				mot,
 				msg->address,
 				msg->buffer,
-				msg->size);
-		return read_bytes;
+				msg->size,
+				&read_bytes);
+		break;
 	case DP_AUX_I2C_WRITE:
 		res = dal_ddc_service_write_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
@@ -137,7 +139,9 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 		 r == DDC_RESULT_SUCESSFULL);
 #endif
 
-	return msg->size;
+	if (res != DDC_RESULT_SUCESSFULL)
+		return -EIO;
+	return read_bytes;
 }
 
 static enum drm_connector_status
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c b/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c
index 49c2face1e7a..ae48d603ebd6 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c
@@ -629,13 +629,14 @@ bool dal_ddc_service_query_ddc_data(
 	return ret;
 }
 
-ssize_t dal_ddc_service_read_dpcd_data(
+enum ddc_result dal_ddc_service_read_dpcd_data(
 	struct ddc_service *ddc,
 	bool i2c,
 	enum i2c_mot_mode mot,
 	uint32_t address,
 	uint8_t *data,
-	uint32_t len)
+	uint32_t len,
+	uint32_t *read)
 {
 	struct aux_payload read_payload = {
 		.i2c_over_aux = i2c,
@@ -652,6 +653,8 @@ ssize_t dal_ddc_service_read_dpcd_data(
 		.mot = mot
 	};
 
+	*read = 0;
+
 	if (len > DEFAULT_AUX_MAX_DATA_SIZE) {
 		BREAK_TO_DEBUGGER();
 		return DDC_RESULT_FAILED_INVALID_OPERATION;
@@ -661,7 +664,8 @@ ssize_t dal_ddc_service_read_dpcd_data(
 		ddc->ctx->i2caux,
 		ddc->ddc_pin,
 		&command)) {
-		return (ssize_t)command.payloads->length;
+		*read = command.payloads->length;
+		return DDC_RESULT_SUCESSFULL;
 	}
 
 	return DDC_RESULT_FAILED_OPERATION;
diff --git a/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h b/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h
index 090b7a8dd67b..30b3a08b91be 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h
@@ -102,13 +102,14 @@ bool dal_ddc_service_query_ddc_data(
 		uint8_t *read_buf,
 		uint32_t read_size);
 
-ssize_t dal_ddc_service_read_dpcd_data(
+enum ddc_result dal_ddc_service_read_dpcd_data(
 		struct ddc_service *ddc,
 		bool i2c,
 		enum i2c_mot_mode mot,
 		uint32_t address,
 		uint8_t *data,
-		uint32_t len);
+		uint32_t len,
+		uint32_t *read);
 
 enum ddc_result dal_ddc_service_write_dpcd_data(
 		struct ddc_service *ddc,
-- 
2.17.0

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

RE: [PATCH] drm/amd/display: Don't return ddc result and read_bytes in same return value

[S, Shirish]

Thanks Harry, it works.

Patch is Reviewed-by: Shirish S <shirish.s@amd.com>

Regards,
Shirish S

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Harry Wentland
Sent: Tuesday, April 24, 2018 8:57 PM
To: amd-gfx@lists.freedesktop.org; S, Shirish <Shirish.S@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; S, Shirish <Shirish.S@amd.com>
Cc: Wentland, Harry <Harry.Wentland@amd.com>
Subject: [PATCH] drm/amd/display: Don't return ddc result and read_bytes in same return value

The two ranges overlap.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---

Thinking of something like this if this works for you.

Harry

 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 20 +++++++++++--------
 .../gpu/drm/amd/display/dc/core/dc_link_ddc.c | 10 +++++++---
 .../gpu/drm/amd/display/dc/inc/dc_link_ddc.h  |  5 +++--
 3 files changed, 22 insertions(+), 13 deletions(-)

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 c3f3028253c3..b8dd7496b7bc 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
@@ -83,21 +83,22 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 	enum i2c_mot_mode mot = (msg->request & DP_AUX_I2C_MOT) ?
 		I2C_MOT_TRUE : I2C_MOT_FALSE;
 	enum ddc_result res;
-	ssize_t read_bytes;
+	uint32_t read_bytes = msg->size;
 
 	if (WARN_ON(msg->size > 16))
 		return -E2BIG;
 
 	switch (msg->request & ~DP_AUX_I2C_MOT) {
 	case DP_AUX_NATIVE_READ:
-		read_bytes = dal_ddc_service_read_dpcd_data(
+		res = dal_ddc_service_read_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
 				false,
 				I2C_MOT_UNDEF,
 				msg->address,
 				msg->buffer,
-				msg->size);
-		return read_bytes;
+				msg->size,
+				&read_bytes);
+		break;
 	case DP_AUX_NATIVE_WRITE:
 		res = dal_ddc_service_write_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
@@ -108,14 +109,15 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 				msg->size);
 		break;
 	case DP_AUX_I2C_READ:
-		read_bytes = dal_ddc_service_read_dpcd_data(
+		res = dal_ddc_service_read_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
 				true,
 				mot,
 				msg->address,
 				msg->buffer,
-				msg->size);
-		return read_bytes;
+				msg->size,
+				&read_bytes);
+		break;
 	case DP_AUX_I2C_WRITE:
 		res = dal_ddc_service_write_dpcd_data(
 				TO_DM_AUX(aux)->ddc_service,
@@ -137,7 +139,9 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
 		 r == DDC_RESULT_SUCESSFULL);
 #endif
 
-	return msg->size;
+	if (res != DDC_RESULT_SUCESSFULL)
+		return -EIO;
+	return read_bytes;
 }
 
 static enum drm_connector_status
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c b/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c
index 49c2face1e7a..ae48d603ebd6 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_ddc.c
@@ -629,13 +629,14 @@ bool dal_ddc_service_query_ddc_data(
 	return ret;
 }
 
-ssize_t dal_ddc_service_read_dpcd_data(
+enum ddc_result dal_ddc_service_read_dpcd_data(
 	struct ddc_service *ddc,
 	bool i2c,
 	enum i2c_mot_mode mot,
 	uint32_t address,
 	uint8_t *data,
-	uint32_t len)
+	uint32_t len,
+	uint32_t *read)
 {
 	struct aux_payload read_payload = {
 		.i2c_over_aux = i2c,
@@ -652,6 +653,8 @@ ssize_t dal_ddc_service_read_dpcd_data(
 		.mot = mot
 	};
 
+	*read = 0;
+
 	if (len > DEFAULT_AUX_MAX_DATA_SIZE) {
 		BREAK_TO_DEBUGGER();
 		return DDC_RESULT_FAILED_INVALID_OPERATION;
@@ -661,7 +664,8 @@ ssize_t dal_ddc_service_read_dpcd_data(
 		ddc->ctx->i2caux,
 		ddc->ddc_pin,
 		&command)) {
-		return (ssize_t)command.payloads->length;
+		*read = command.payloads->length;
+		return DDC_RESULT_SUCESSFULL;
 	}
 
 	return DDC_RESULT_FAILED_OPERATION;
diff --git a/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h b/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h
index 090b7a8dd67b..30b3a08b91be 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/dc_link_ddc.h
@@ -102,13 +102,14 @@ bool dal_ddc_service_query_ddc_data(
 		uint8_t *read_buf,
 		uint32_t read_size);
 
-ssize_t dal_ddc_service_read_dpcd_data(
+enum ddc_result dal_ddc_service_read_dpcd_data(
 		struct ddc_service *ddc,
 		bool i2c,
 		enum i2c_mot_mode mot,
 		uint32_t address,
 		uint8_t *data,
-		uint32_t len);
+		uint32_t len,
+		uint32_t *read);
 
 enum ddc_result dal_ddc_service_write_dpcd_data(
 		struct ddc_service *ddc,
-- 
2.17.0

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