diff mbox

[3/7] drm/i915: Don't return error on sink crc stop.

Message ID 1437694550-5667-2-git-send-email-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi July 23, 2015, 11:35 p.m. UTC
If we got to the point where we are trying to stop sink CRC
the main output of this function was already gotten properly,
so don't return the error and let userspace use the crc data.

Let's replace the errnos returns with some log messages.

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

Comments

Rafael Antognolli July 24, 2015, 6:22 p.m. UTC | #1
On Thu, Jul 23, 2015 at 04:35:46PM -0700, Rodrigo Vivi wrote:
> If we got to the point where we are trying to stop sink CRC
> the main output of this function was already gotten properly,
> so don't return the error and let userspace use the crc data.
> 
> Let's replace the errnos returns with some log messages.

So, up to this commit, there's no way to know that an error has ocurred
and the next CRC calculation can go wrong.

I know that in a follow up patch this is fixed by trying to stop the
calculation at the beginning too, but just pointing out that this one
specifically has this problem.

Not sure if this is a problem though, since the patches are submitted
together. If not, then

Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>


> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 70a4a37..44f8a32 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4021,12 +4021,12 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  
>  stop:
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		ret = -EIO;
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  		goto out;
>  	}
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
>  			       buf & ~DP_TEST_SINK_START) < 0) {
> -		ret = -EIO;
> +		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
>  		goto out;
>  	}
>  out:
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rafael
Rodrigo Vivi July 24, 2015, 6:33 p.m. UTC | #2
On Fri, Jul 24, 2015 at 11:25 AM Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> On Thu, Jul 23, 2015 at 04:35:46PM -0700, Rodrigo Vivi wrote:
> > If we got to the point where we are trying to stop sink CRC
> > the main output of this function was already gotten properly,
> > so don't return the error and let userspace use the crc data.
> >
> > Let's replace the errnos returns with some log messages.
>
> So, up to this commit, there's no way to know that an error has ocurred
> and the next CRC calculation can go wrong.
>
> I know that in a follow up patch this is fixed by trying to stop the
> calculation at the beginning too, but just pointing out that this one
> specifically has this problem.
>

Actually it isn't a problem if crc calculation continue enabled. Mainly
with the mask s/0x7/0xf fix.
But it is good to disable and force counter reset anyway. (the following
patch you mentioned)

This patch here is just to highlight that errors during read has more
priority than errors on stopping crc.

Another way would be to create a stop_ret and
if (!ret and stop_ret)
     ret = stop_ret;

But I decided this cleaned version would be better.


>
> Not sure if this is a problem though, since the patches are submitted
> together. If not, then
>
> Reviewed-by: Rafael Antognolli <rafael.antognolli@intel.com>
>

Thanks!


>
>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> > index 70a4a37..44f8a32 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4021,12 +4021,12 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp,
> u8 *crc)
> >
> >  stop:
> >       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -             ret = -EIO;
> > +             DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >               goto out;
> >       }
> >       if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> >                              buf & ~DP_TEST_SINK_START) < 0) {
> > -             ret = -EIO;
> > +             DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >               goto out;
> >       }
> >  out:
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Rafael
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 70a4a37..44f8a32 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4021,12 +4021,12 @@  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 
 stop:
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		ret = -EIO;
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		goto out;
 	}
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
 			       buf & ~DP_TEST_SINK_START) < 0) {
-		ret = -EIO;
+		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
 		goto out;
 	}
 out: