Re: [PATCH 28/31] drm/i915: Make Sink crc calculation waiting for counter to reset.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 10, 2015 at 07:49:51PM -0200, Paulo Zanoni wrote:
> 2015-11-10 18:31 GMT-02:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> >> According to VESA DP spec TEST_CRC_COUNT (Bits 3:0) at
> >> TEST_SINK_MISC (00246h) is "Reset to 0 when TEST_SINK bit 0 = 0;
> >>
> >> So let's give few vblanks so we are really sure that this counter
> >> is really zeroed on the next sink_crc read.
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++++++++++++-
> >>  1 file changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index c0fa90a..5d810cd 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -3806,6 +3806,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>         struct intel_crtc *intel_crtc = to_intel_crtc(dig_port->base.base.crtc);
> >>         u8 buf;
> >>         int ret = 0;
> >> +       int count = 0;
> >> +       int attempts = 10;
> >>
> >>         if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> >>                 DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> >> @@ -3820,7 +3822,22 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp)
> >>                 goto out;
> >>         }
> >>
> >> -       intel_wait_for_vblank(dev, intel_crtc->pipe);
> >> +       do {
> >> +               intel_wait_for_vblank(dev, intel_crtc->pipe);
> >> +
> >> +               if (drm_dp_dpcd_readb(&intel_dp->aux,
> >> +                                     DP_TEST_SINK_MISC, &buf) < 0) {
> >> +                       ret = -EIO;
> >> +                       goto out;
> >
> > This "goto out" will make sink_crc.started remain as true even though
> > we already sent the DPCD message telling it to stop, and it
> > acknowledged our message. And it won't even print stuff on dmesg. I
> > guess I'd probably write something on dmesg and flip started to false.
> 
> Now I see that patch 30 deals with this issue.
> 
> >
> >> +               }
> >> +               count = buf & DP_TEST_COUNT_MASK;
> >> +       } while (--attempts && count);
> >> +
> >> +       if (attempts == 0) {
> >> +               DRM_ERROR("TIMEOUT: Sink CRC counter is not zeroed\n");
> >
> > The other errors are all DRM_DEBUG_KMS. On one hand we can't do
> > anything about them since they're most likely panel errors so
> > DRM_ERROR doesn't look good. On the other hand normal users are not
> > going to ever run this code, and DRM_ERROR may make us - and our
> > testing robots - notice the possible failures, so maybe DRM_ERROR is
> > the way to go here. Anyway, we should be consistent regardless of the
> > decision.
> >
> > Besides, at intel_dp_sink_crc_start(), we read the last_count, but
> > it's supposed to be zero. Can't we use a check for this there too?
> > Maybe just an informative DRM_DEBUG_KMS("this was supposed to be zero
> > but it's not\n") without really returning.
> 
> This is addressed by patch 29.
> 
> >
> > Everything else looks good.
> 
> So with or without the changes between the log level of the messages
> (since end users shouldn't be running them):
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I also vote that we merge 27, 28, 29 and 30 right now since they don't
> require patches 1-26. The only conflict is the rename of the IPS
> functions, and this can be easily fixed in the patch file.

Good idea, all 4 pulled into dinq. Rodrigo, is this all we need to make
sink CRC reliable? Or is the read_wake stuff still needed?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx