[Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Feb 18 17:57:19 UTC 2019
On Fri, Feb 15, 2019 at 09:43:37PM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2019-02-15 at 23:34 +0200, Ville Syrjälä wrote:
> > On Fri, Feb 15, 2019 at 01:06:32PM -0800, Dhinakaran Pandiyan wrote:
> > > On Fri, 2019-02-15 at 14:47 +0200, Ville Syrjälä wrote:
> > > > On Thu, Feb 14, 2019 at 06:26:29PM -0800, Dhinakaran Pandiyan
> > > > wrote:
> > > > > On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > >
> > > > > > DP CRCs don't really work on g4x. If you want any CRCs on DP
> > > > > > you
> > > > > > must
> > > > > > select the CRC source before the port is enabled, otherwise
> > > > > > the
> > > > > > CRC
> > > > > > source select bits simply ignore any writes to them. And once
> > > > > > the
> > > > > > port
> > > > > > is enabled we mustn't change the CRC source select until the
> > > > > > port
> > > > > > is
> > > > > > disabled. That almost works, but not quite :( Eventually the
> > > > > > CRC
> > > > > > source
> > > > > > select bits get permanently stuck one way or the other, and
> > > > > > after
> > > > > > that
> > > > > > a reboot (or possibly a display reset) is needed to get
> > > > > > working
> > > > > > CRCs
> > > > > > on that pipe (not matter which CRC source we try to use).
> > > > > >
> > > > > > Additionally the DFT scrambler reset bits we're trying to use
> > > > > > don't
> > > > > > seem to exist on g4x. There are some potentially relevant
> > > > > > looking
> > > > > > bits
> > > > > > in the pipe registers, but when I tried it I got stable
> > > > > > looking
> > > > > > CRCs
> > > > > > without setting any bits for this.
> > > > > >
> > > > > > If there is a way to make DP CRCs work reliably on g4x, I
> > > > > > wasn't
> > > > > > able to find it. So let's just remove the broken code we
> > > > > > have.
> > > > >
> > > > >
> > > > > I think we can modify i9xx_pipe_crc_auto_source() to pick
> > > > > "pipe"
> > > > > CRC
> > > > > when userspace selects "auto" and the output is DP/eDP.
> > > >
> > > > Nope. Spec says:
> > > > "Pipe CRC should not be run when Display Port or TV is enabled on
> > > > this
> > > > pipe."
> > > >
> > > > and
> > > >
> > > > "CRC Source Select: These bits select the source of the data to
> > > > put
> > > > into
> > > > the CRC logic.
> > > > 000: Pipe A (Not available when DisplayPort or TV is enabled on
> > > > this
> > > > pipe)"
> > > >
> > >
> > > After digging through some old specs, I do see this restriction for
> > > gen-4 and VLV, but for some reason not for gen-3 or CHV.
> >
> > gen3 predates DP (g4x being the first platform that has it). I don't
> > think SDVO->DP was ever a thing. SDVO->HDMI did happen but even that
> > one is quite rare.
>
> TV? I see TV initialization for a couple of gen-3 platforms but the
> spec does not say that pipe CRCs are not available.
Could just be an omission in the spec. I don't think I actually
tested to see what happens when you try to use the pipe CRC with the
TV encoder. Presumably it might give you something useful, but it
certainly wouldn't account for anything done by the TV encoder.
Not that we normally care about that stuff anyway.
> >
> > The display engine side of CHV is 99.9% VLV, with a few extra
> > bits and pieces glued on top.
>
> Thanks for the clarification, the CHV spec for some reason make it a
> point to specify VLV in paranthesis
They basically just did 'cp VLVspec CHVspec', and then edited
it minimally. So you should generally interpret the "DevVLV*"
to mean "VLV/CHV".
>
> 0000: Pipe C (Not available when DisplayPort or TV is enabled on this
> pipe) [VLV]
>
> >
> > >
> > > There is no good choice for "auto" other than DP and since DP does
> > > not
> > > work, returning -EINVAL makes sense.
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > >
> > > > Though I must admit I've never actually tried it to see what
> > > > actually
> > > > happens.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 80 ++++-------------
> > > > > > ----
> > > > > > ----
> > > > > > --
> > > > > > 1 file changed, 11 insertions(+), 69 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > index fe0ff89b980b..66bb7b031537 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > > > @@ -191,8 +191,6 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > enum intel_pipe_crc_source
> > > > > > *source,
> > > > > > u32 *val)
> > > > > > {
> > > > > > - bool need_stable_symbols = false;
> > > > > > -
> > > > > > if (*source == INTEL_PIPE_CRC_SOURCE_AUTO) {
> > > > > > int ret = i9xx_pipe_crc_auto_source(dev_priv,
> > > > > > pipe,
> > > > > > source);
> > > > > > if (ret)
> > > > > > @@ -208,56 +206,23 @@ static int i9xx_pipe_crc_ctl_reg(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > return -EINVAL;
> > > > > > *val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_TV_PRE;
> > > > > > break;
> > > > > > - case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > - if (!IS_G4X(dev_priv))
> > > > > > - return -EINVAL;
> > > > > > - *val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_DP_B_G4X;
> > > > > > - need_stable_symbols = true;
> > > > > > - break;
> > > > > > - case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > - if (!IS_G4X(dev_priv))
> > > > > > - return -EINVAL;
> > > > > > - *val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_DP_C_G4X;
> > > > > > - need_stable_symbols = true;
> > > > > > - break;
> > > > > > - case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > - if (!IS_G4X(dev_priv))
> > > > > > - return -EINVAL;
> > > > > > - *val = PIPE_CRC_ENABLE |
> > > > > > PIPE_CRC_SOURCE_DP_D_G4X;
> > > > > > - need_stable_symbols = true;
> > > > > > - break;
> > > > > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > > *val = 0;
> > > > > > break;
> > > > > > default:
> > > > > > + /*
> > > > > > + * The DP CRC source doesn't work on g4x.
> > > > > > + * It can be made to work to some degree by
> > > > > > selecting
> > > > > > + * the correct CRC source before the port is
> > > > > > enabled,
> > > > > > + * and not touching the CRC source bits again
> > > > > > until
> > > > > > + * the port is disabled. But even then the bits
> > > > > > + * eventually get stuck and a reboot is needed
> > > > > > to get
> > > > > > + * working CRCs on the pipe again. Let's simply
> > > > > > + * refuse to use DP CRCs on g4x.
> > > > > > + */z
> > > > > > return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > - /*
> > > > > > - * When the pipe CRC tap point is after the transcoders
> > > > > > we need
> > > > > > - * to tweak symbol-level features to produce a
> > > > > > deterministic
> > > > > > series of
> > > > > > - * symbols for a given frame. We need to reset those
> > > > > > features
> > > > > > only once
> > > > > > - * a frame (instead of every nth symbol):
> > > > > > - * - DC-balance: used to ensure a better clock
> > > > > > recovery from
> > > > > > the data
> > > > > > - * link (SDVO)
> > > > > > - * - DisplayPort scrambling: used for EMI reduction
> > > > > > - */
> > > > > > - if (need_stable_symbols) {
> > > > > > - u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > -
> > > > > > - WARN_ON(!IS_G4X(dev_priv));
> > > > > > -
> > > > > > - I915_WRITE(PORT_DFT_I9XX,
> > > > > > - I915_READ(PORT_DFT_I9XX) |
> > > > > > DC_BALANCE_RESET);
> > > > > > -
> > > > > > - if (pipe == PIPE_A)
> > > > > > - tmp |= PIPE_A_SCRAMBLE_RESET;
> > > > > > - else
> > > > > > - tmp |= PIPE_B_SCRAMBLE_RESET;
> > > > > > -
> > > > > > - I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > - }
> > > > > > -
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > @@ -282,24 +247,6 @@ static void
> > > > > > vlv_undo_pipe_scramble_reset(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > if (!(tmp & PIPE_SCRAMBLE_RESET_MASK))
> > > > > > tmp &= ~DC_BALANCE_RESET_VLV;
> > > > > > I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > -
> > > > > > -}
> > > > > > -
> > > > > > -static void g4x_undo_pipe_scramble_reset(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv,
> > > > > > - enum pipe pipe)
> > > > > > -{
> > > > > > - u32 tmp = I915_READ(PORT_DFT2_G4X);
> > > > > > -
> > > > > > - if (pipe == PIPE_A)
> > > > > > - tmp &= ~PIPE_A_SCRAMBLE_RESET;
> > > > > > - else
> > > > > > - tmp &= ~PIPE_B_SCRAMBLE_RESET;
> > > > > > - I915_WRITE(PORT_DFT2_G4X, tmp);
> > > > > > -
> > > > > > - if (!(tmp & PIPE_SCRAMBLE_RESET_MASK)) {
> > > > > > - I915_WRITE(PORT_DFT_I9XX,
> > > > > > - I915_READ(PORT_DFT_I9XX) &
> > > > > > ~DC_BALANCE_RESET);
> > > > > > - }
> > > > > > }
> > > > > >
> > > > > > static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source
> > > > > > *source,
> > > > > > @@ -485,9 +432,6 @@ static int i9xx_crc_source_valid(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > > switch (source) {
> > > > > > case INTEL_PIPE_CRC_SOURCE_PIPE:
> > > > > > case INTEL_PIPE_CRC_SOURCE_TV:
> > > > > > - case INTEL_PIPE_CRC_SOURCE_DP_B:
> > > > > > - case INTEL_PIPE_CRC_SOURCE_DP_C:
> > > > > > - case INTEL_PIPE_CRC_SOURCE_DP_D:
> > > > > > case INTEL_PIPE_CRC_SOURCE_NONE:
> > > > > > return 0;
> > > > > > default:
> > > > > > @@ -612,9 +556,7 @@ int intel_crtc_set_crc_source(struct
> > > > > > drm_crtc
> > > > > > *crtc, const char *source_name)
> > > > > > POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > > >
> > > > > > if (!source) {
> > > > > > - if (IS_G4X(dev_priv))
> > > > > > - g4x_undo_pipe_scramble_reset(dev_priv,
> > > > > > crtc-
> > > > > > > index);
> > > > > >
> > > > > > - else if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > + if (IS_VALLEYVIEW(dev_priv) ||
> > > > > > IS_CHERRYVIEW(dev_priv))
> > > > > > vlv_undo_pipe_scramble_reset(dev_priv,
> > > > > > crtc-
> > > > > > > index);
> > > > > >
> > > > > > else if ((IS_HASWELL(dev_priv) ||
> > > > > > IS_BROADWELL(dev_priv)) && crtc-
> > > > > > >index ==
> > > > > > PIPE_A)
> > > >
> > > >
> >
> >
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list