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 
---
  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

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 
---
  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)

[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 
> ---
>  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