All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
@ 2014-09-29 22:29 Rodrigo Vivi
  2014-09-29 22:39 ` Todd Previte
  2014-09-30 20:36 ` Daniel Vetter
  0 siblings, 2 replies; 6+ messages in thread
From: Rodrigo Vivi @ 2014-09-29 22:29 UTC (permalink / raw)
  To: intel-gfx; +Cc: Rodrigo Vivi

Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
reserved reading all 0s. But when reviewing my latest changes on sink crc
Todd warned me that on new specs we have other valid bits on this reg that we
might want to preserve.

Cc: Todd Previte <tprevite@gmail.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b8699b0..7d5fa2f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
 		return -ENOTTY;
 
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       DP_TEST_SINK_START) < 0)
+				buf | DP_TEST_SINK_START) < 0)
 		return -EIO;
 
 	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf);
@@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
 		return -EIO;
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
+			buf & ~DP_TEST_SINK_START);
+
 	return 0;
 }
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
  2014-09-29 22:29 [PATCH] drm/i915: preserve other DP_TEST_SINK bits Rodrigo Vivi
@ 2014-09-29 22:39 ` Todd Previte
  2014-09-30  7:42   ` Daniel Vetter
  2014-09-30 20:36 ` Daniel Vetter
  1 sibling, 1 reply; 6+ messages in thread
From: Todd Previte @ 2014-09-29 22:39 UTC (permalink / raw)
  To: 'Rodrigo Vivi', intel-gfx

Hi Rodrigo,

Looks good. Only thing that needs to be removed is that extra blank line
between the last part of the function and the return statement. Otherwise...

Reviewed-by: Todd Previte <tprevite@gmail.com>

-----Original Message-----
From: Rodrigo Vivi [mailto:rodrigo.vivi@intel.com] 
Sent: Monday, September 29, 2014 3:30 PM
To: intel-gfx@lists.freedesktop.org
Cc: Rodrigo Vivi; Todd Previte
Subject: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.

Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
reserved reading all 0s. But when reviewing my latest changes on sink crc
Todd warned me that on new specs we have other valid bits on this reg that
we might want to preserve.

Cc: Todd Previte <tprevite@gmail.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
*crc)
 	if (!(buf & DP_TEST_CRC_SUPPORTED))
 		return -ENOTTY;
 
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       DP_TEST_SINK_START) < 0)
+				buf | DP_TEST_SINK_START) < 0)
 		return -EIO;
 
 	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@
-3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
*crc)
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
 		return -EIO;
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
+			buf & ~DP_TEST_SINK_START);
+
 	return 0;
 }
 
--
1.9.3

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
  2014-09-29 22:39 ` Todd Previte
@ 2014-09-30  7:42   ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-09-30  7:42 UTC (permalink / raw)
  To: Todd Previte; +Cc: intel-gfx, 'Rodrigo Vivi'

On Mon, Sep 29, 2014 at 03:39:06PM -0700, Todd Previte wrote:
> Hi Rodrigo,
> 
> Looks good. Only thing that needs to be removed is that extra blank line
> between the last part of the function and the return statement. Otherwise...

Looking around in our driver the return statement at the end of the
function is often on a separate line.
> 
> Reviewed-by: Todd Previte <tprevite@gmail.com>

Queued for -next, thanks for the patch.
> 
> -----Original Message-----
> From: Rodrigo Vivi [mailto:rodrigo.vivi@intel.com] 
> Sent: Monday, September 29, 2014 3:30 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Rodrigo Vivi; Todd Previte
> Subject: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
> 
> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
> reserved reading all 0s. But when reviewing my latest changes on sink crc
> Todd warned me that on new specs we have other valid bits on this reg that
> we might want to preserve.

Why was this not mentioned in the review to that patch? I've
double-checked Todd's reply and I've only found a "lgtm, r-b".

If you spot something odd, or something that should be addressed in a
follow-up patch, that should be part of your review reply - review is much
more than rubber-stamping with r-b tags.

And if that review happened offline or in Portland, pls always try to
summarize the main points discussed in the patch or reply for the benefit
of everyone else. We're a globally distributed team, so active and
inclusive communication is absolutely key.

Thanks, Daniel

> 
> Cc: Todd Previte <tprevite@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
> *crc)
>  	if (!(buf & DP_TEST_CRC_SUPPORTED))
>  		return -ENOTTY;
>  
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       DP_TEST_SINK_START) < 0)
> +				buf | DP_TEST_SINK_START) < 0)
>  		return -EIO;
>  
>  	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@
> -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
> *crc)
>  	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>  		return -EIO;
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> +			buf & ~DP_TEST_SINK_START);
> +
>  	return 0;
>  }
>  
> --
> 1.9.3
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
  2014-09-29 22:29 [PATCH] drm/i915: preserve other DP_TEST_SINK bits Rodrigo Vivi
  2014-09-29 22:39 ` Todd Previte
@ 2014-09-30 20:36 ` Daniel Vetter
  2014-09-30 20:41   ` Rodrigo Vivi
  1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-09-30 20:36 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

On Mon, Sep 29, 2014 at 06:29:52PM -0400, Rodrigo Vivi wrote:
> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
> reserved reading all 0s. But when reviewing my latest changes on sink crc
> Todd warned me that on new specs we have other valid bits on this reg that we
> might want to preserve.
> 
> Cc: Todd Previte <tprevite@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b8699b0..7d5fa2f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	if (!(buf & DP_TEST_CRC_SUPPORTED))
>  		return -ENOTTY;
>  
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,

With this we no longer check the first aux transaction for errors, and
maybe we should even check them all?
-Daniel

> -			       DP_TEST_SINK_START) < 0)
> +				buf | DP_TEST_SINK_START) < 0)
>  		return -EIO;
>  
>  	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf);
> @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>  		return -EIO;
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> +			buf & ~DP_TEST_SINK_START);
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
  2014-09-30 20:36 ` Daniel Vetter
@ 2014-09-30 20:41   ` Rodrigo Vivi
  2014-10-01  8:06     ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Rodrigo Vivi @ 2014-09-30 20:41 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi

Please don't forget the drm/i915: Fix Sink CRC

That is the important one that fix crc calculation and will allow me
to remove the extra wait for vblanks on tests besides making tests
more stable.


On Tue, Sep 30, 2014 at 1:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 29, 2014 at 06:29:52PM -0400, Rodrigo Vivi wrote:
>> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
>> reserved reading all 0s. But when reviewing my latest changes on sink crc
>> Todd warned me that on new specs we have other valid bits on this reg that we
>> might want to preserve.
>>
>> Cc: Todd Previte <tprevite@gmail.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index b8699b0..7d5fa2f 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>>       if (!(buf & DP_TEST_CRC_SUPPORTED))
>>               return -ENOTTY;
>>
>> +     drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
>>       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>
> With this we no longer check the first aux transaction for errors, and
> maybe we should even check them all?

That is true. Do you prefer a v2 or top fix?
And what do you prefer? avoid the check at all or check on the first
aux transaction?


> -Daniel
>
>> -                            DP_TEST_SINK_START) < 0)
>> +                             buf | DP_TEST_SINK_START) < 0)
>>               return -EIO;
>>
>>       drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf);
>> @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>>       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
>>               return -EIO;
>>
>> -     drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
>> +     drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
>> +     drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>> +                     buf & ~DP_TEST_SINK_START);
>> +
>>       return 0;
>>  }
>>
>> --
>> 1.9.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits.
  2014-09-30 20:41   ` Rodrigo Vivi
@ 2014-10-01  8:06     ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-10-01  8:06 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi

On Tue, Sep 30, 2014 at 01:41:20PM -0700, Rodrigo Vivi wrote:
> Please don't forget the drm/i915: Fix Sink CRC
> 
> That is the important one that fix crc calculation and will allow me
> to remove the extra wait for vblanks on tests besides making tests
> more stable.
> 
> 
> On Tue, Sep 30, 2014 at 1:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Sep 29, 2014 at 06:29:52PM -0400, Rodrigo Vivi wrote:
> >> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits
> >> reserved reading all 0s. But when reviewing my latest changes on sink crc
> >> Todd warned me that on new specs we have other valid bits on this reg that we
> >> might want to preserve.
> >>
> >> Cc: Todd Previte <tprevite@gmail.com>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index b8699b0..7d5fa2f 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >>       if (!(buf & DP_TEST_CRC_SUPPORTED))
> >>               return -ENOTTY;
> >>
> >> +     drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
> >>       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> >
> > With this we no longer check the first aux transaction for errors, and
> > maybe we should even check them all?
> 
> That is true. Do you prefer a v2 or top fix?
> And what do you prefer? avoid the check at all or check on the first
> aux transaction?

Fixup patch is ok, I've kept your patch here. And I guess if you bother
you should add checks with return -EIO to all of them, since they could
all fail. Even if it's just for consistency.
-Daniel

> 
> 
> > -Daniel
> >
> >> -                            DP_TEST_SINK_START) < 0)
> >> +                             buf | DP_TEST_SINK_START) < 0)
> >>               return -EIO;
> >>
> >>       drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf);
> >> @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
> >>       if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0)
> >>               return -EIO;
> >>
> >> -     drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0);
> >> +     drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf);
> >> +     drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> >> +                     buf & ~DP_TEST_SINK_START);
> >> +
> >>       return 0;
> >>  }
> >>
> >> --
> >> 1.9.3
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-01  8:06 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-29 22:29 [PATCH] drm/i915: preserve other DP_TEST_SINK bits Rodrigo Vivi
2014-09-29 22:39 ` Todd Previte
2014-09-30  7:42   ` Daniel Vetter
2014-09-30 20:36 ` Daniel Vetter
2014-09-30 20:41   ` Rodrigo Vivi
2014-10-01  8:06     ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.