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@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?
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
