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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Wed, 2015-11-18 at 11:25 +0100, Daniel Vetter wrote:
> 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?

On SKL and KBL the read_wake is still needed. or we treat that case
where we read the message size 0 what is forbidden and retry or return
EBUSY to let drm level retrie when we have no idea what is happening
with hardware...



> -Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx