Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
On Mon, 2019-02-18 at 19:57 +0200, Ville Syrjälä wrote: > On Fri, Feb 15, 2019 at 09:43:37PM +, 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ä > > > > > > > > > > > > > > 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. PSR2 might be one of those cases that makes encoder CRCs interesting - for e.g., compare DDI CRC of a PSR2 SU update region against a reference. I don't know how we can generate a reference CRC for a SU update region though. > > > > > > > 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". Got it, thanks for explaining :) -DK > > > > > : 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 > > > > > > > > > > > > > Though I must admit I've never actually tried it to see what > > > > > actually > > > > > happens. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä < > > > > > > > ville.syrj...@linux.intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 80 - > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > 1 file changed, 11 insertions(+), 69 delet
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
On Fri, Feb 15, 2019 at 09:43:37PM +, 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ä
> > > > > >
> > > > > > 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".
>
> : 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
> > >
> > > > Though I must admit I've never actually tried it to see what
> > > > actually
> > > > happens.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Ville Syrjälä
> > > > > > ---
> > > > > > 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);
> >
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
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ä
> > > > >
> > > > > 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.
>
> 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
: 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
> >
> > > Though I must admit I've never actually tried it to see what
> > > actually
> > > happens.
> > >
> > > >
> > > >
> > > > >
> > > > > Signed-off-by: Ville Syrjälä
> > > > > ---
> > > > > 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;
> > > > > -
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
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ä
> > > >
> > > > 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.
The display engine side of CHV is 99.9% VLV, with a few extra
bits and pieces glued on top.
>
> There is no good choice for "auto" other than DP and since DP does not
> work, returning -EINVAL makes sense.
> Reviewed-by: Dhinakaran Pandiyan
>
> > Though I must admit I've never actually tried it to see what actually
> > happens.
> >
> > >
> > >
> > > >
> > > > Signed-off-by: Ville Syrjälä
> > > > ---
> > > > 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 select
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
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ä
> > >
> > > 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.
There is no good choice for "auto" other than DP and since DP does not
work, returning -EINVAL makes sense.
Reviewed-by: Dhinakaran Pandiyan
> Though I must admit I've never actually tried it to see what actually
> happens.
>
> >
> >
> > >
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > 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 sy
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
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ä
> >
> > 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)"
Though I must admit I've never actually tried it to see what actually
happens.
>
>
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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)
> > -
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
On Thu, 2019-02-14 at 21:22 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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.
>
> Signed-off-by: Ville Syrjälä
> ---
> 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_pri
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
On Thu, Feb 14, 2019 at 12:38:22PM -0800, Rodrigo Vivi wrote:
> On Thu, Feb 14, 2019 at 09:22:18PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä
> >
> > 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.
> >
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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.
> > +*/
> > return -EINVAL;
>
> is this the right return now? maybe ENOENT?
> (just brainstorming without looking to igt tests)
We return -EINVAL for all other unsupported sources, so this
seems consistent.
>
> But I know how terrible unreliable crcs are and this patch
> looks the right way, so:
>
> Reviewed-by: Rodrigo Vivi
>
>
>
> > }
> >
> > - /*
> > -* 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;
> > }
> >
>
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
On Thu, Feb 14, 2019 at 09:22:18PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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.
>
> Signed-off-by: Ville Syrjälä
> ---
> 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.
> + */
> return -EINVAL;
is this the right return now? maybe ENOENT?
(just brainstorming without looking to igt tests)
But I know how terrible unreliable crcs are and this patch
looks the right way, so:
Reviewed-by: Rodrigo Vivi
> }
>
> - /*
> - * 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);
> -
> -}
> -
> -stat
[Intel-gfx] [PATCH 3/4] drm/i915: Remove the broken DP CRC support for g4x
From: Ville Syrjälä
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.
Signed-off-by: Ville Syrjälä
---
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.
+*/
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
