On Tue, Nov 10, 2015 at 07:49:51PM -0200, Paulo Zanoni wrote:
> 2015-11-10 18:31 GMT-02:00 Paulo Zanoni <przanoni@xxxxxxxxx>:
> > 2015-11-05 16:50 GMT-02:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>:
> >> 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@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
