[PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Todd Previte]

These counters are used for Displayort complinace testing to detect error conditions
when executing certain compliance tests. Currently these are used in the EDID tests
to determine if the video mode needs to be set to the preferred mode or the failsafe
mode.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 2 ++
 include/drm/drm_dp_helper.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 08e33b8..8353051 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_NACK:
 			DRM_DEBUG_KMS("I2C nack\n");
+			aux->i2c_nack_count++;
 			return -EREMOTEIO;
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
+			aux->i2c_defer_count++;
 			usleep_range(400, 500);
 			continue;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8edeed0..45f3ee8 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -551,6 +551,7 @@ struct drm_dp_aux {
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
+	uint8_t i2c_nack_count, i2c_defer_count;
 };
 
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
-- 
1.9.1

Re: [Intel-gfx] [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Paulo Zanoni]

2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> These counters are used for Displayort complinace testing to detect error conditions
> when executing certain compliance tests. Currently these are used in the EDID tests
> to determine if the video mode needs to be set to the preferred mode or the failsafe
> mode.
>
> Cc: dri-devel@lists.freedesktop.org

I see that this patch and a few others in your series still have
unaddressed/unanswered review comments, given on the first time you
sent the patches. Please take a look at them.

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 ++
>  include/drm/drm_dp_helper.h     | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 08e33b8..8353051 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>
>                 case DP_AUX_I2C_REPLY_NACK:
>                         DRM_DEBUG_KMS("I2C nack\n");
> +                       aux->i2c_nack_count++;
>                         return -EREMOTEIO;
>
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
> +                       aux->i2c_defer_count++;
>                         usleep_range(400, 500);
>                         continue;
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8edeed0..45f3ee8 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -551,6 +551,7 @@ struct drm_dp_aux {
>         struct mutex hw_mutex;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
> +       uint8_t i2c_nack_count, i2c_defer_count;
>  };
>
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [Intel-gfx] [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Todd Previte]

To address previous feedback, I'll quote below and answer.

>It would be nice if you could cite on the commit message the name of
>the specification and the name of the test(s) that use it.

Done. For reference, in the Displayport Link CTS spec, tests 4.2.2.4 and 
4.2.2.5 are the ones that use these, specifically.

>Does it really need to be uint8_t? I see on patch 7 that you don't
>really write this value to a place that only accepts uint8_t-sized
>arguments, so I fear that if we get 256 NACKs or DEFERs we may end up
>doing the wrong thing.

There are no compliance tests that are concerned with the number of 
native DEFERs or NACKs received, hence why they are not addressed here.
There's no requirement for the size of this value and I selected uint8_t 
as the smallest reasonable size for it. It's only used to count the 
number of NACKs and DEFERs during compliance testing and it gets reset 
to 0 at the beginning of the test block where it's used in a later 
patch. It's unlikely that a case would occur during this compliance test 
where exactly 256 NACKs or DEFERs occur, but your point is well-taken. 
I'll make them uint32s and be done with it. I don't think 6 extra bytes 
is going to run the kernel out of memory. :)

>Also, why don't we need to count the native NACKs and DEFERs?

There are no compliance tests that are concerned with the number of 
native DEFERs or NACKs received.

New patch will be here shortly.

-T

On 10/21/2014 10:10 AM, Paulo Zanoni wrote:
> 2014-10-09 12:38 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> These counters are used for Displayort complinace testing to detect error conditions
>> when executing certain compliance tests. Currently these are used in the EDID tests
>> to determine if the video mode needs to be set to the preferred mode or the failsafe
>> mode.
>>
>> Cc: dri-devel@lists.freedesktop.org
> I see that this patch and a few others in your series still have
> unaddressed/unanswered review comments, given on the first time you
> sent the patches. Please take a look at them.
>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/drm_dp_helper.c | 2 ++
>>   include/drm/drm_dp_helper.h     | 1 +
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 08e33b8..8353051 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>>
>>                  case DP_AUX_I2C_REPLY_NACK:
>>                          DRM_DEBUG_KMS("I2C nack\n");
>> +                       aux->i2c_nack_count++;
>>                          return -EREMOTEIO;
>>
>>                  case DP_AUX_I2C_REPLY_DEFER:
>>                          DRM_DEBUG_KMS("I2C defer\n");
>> +                       aux->i2c_defer_count++;
>>                          usleep_range(400, 500);
>>                          continue;
>>
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 8edeed0..45f3ee8 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -551,6 +551,7 @@ struct drm_dp_aux {
>>          struct mutex hw_mutex;
>>          ssize_t (*transfer)(struct drm_dp_aux *aux,
>>                              struct drm_dp_aux_msg *msg);
>> +       uint8_t i2c_nack_count, i2c_defer_count;
>>   };
>>
>>   ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [Intel-gfx] [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Daniel Vetter]

On Tue, Nov 04, 2014 at 03:12:27PM -0700, Todd Previte wrote:
> >Does it really need to be uint8_t? I see on patch 7 that you don't
> >really write this value to a place that only accepts uint8_t-sized
> >arguments, so I fear that if we get 256 NACKs or DEFERs we may end up
> >doing the wrong thing.
> 
> There are no compliance tests that are concerned with the number of native
> DEFERs or NACKs received, hence why they are not addressed here.
> There's no requirement for the size of this value and I selected uint8_t as
> the smallest reasonable size for it. It's only used to count the number of
> NACKs and DEFERs during compliance testing and it gets reset to 0 at the
> beginning of the test block where it's used in a later patch. It's unlikely
> that a case would occur during this compliance test where exactly 256 NACKs
> or DEFERs occur, but your point is well-taken. I'll make them uint32s and be
> done with it. I don't think 6 extra bytes is going to run the kernel out of
> memory. :)

Du to struct alignement rules a lone uint8_t will actually occupy as much
as a plain unsigned. Also uint8_t in drivers has a bit the air of a
carefully picked size, usually in relation to some hw limit (e.g. register
size).

Imo just go with unsigned, it's clearer in intent and actually doesn't use
more space.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel