On Fri, Jun 15, 2018 at 02:51:06PM -0700, Lucas De Marchi wrote:
> On Fri, Jun 15, 2018 at 08:58:28PM +0300, Ville Syrjälä wrote:
> > On Wed, May 23, 2018 at 11:04:35AM -0700, Lucas De Marchi wrote:
> > > This became dead code with commit 309bd8ed464f ("drm/i915: Reinstate
> > > GMBUS and AUX interrupts on gen4/g4x").
> > >
> > > v2: Move comment about HW behavior to where decision is made to enable
> > > MSI (Ville).
> > >
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.c | 6 ++++++
> > > drivers/gpu/drm/i915/i915_drv.h | 10 ----------
> > > drivers/gpu/drm/i915/intel_dp.c | 22 +++++++---------------
> > > drivers/gpu/drm/i915/intel_drv.h | 1 -
> > > drivers/gpu/drm/i915/intel_psr.c | 2 +-
> > > 5 files changed, 14 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 9c449b8d8eab..a1461de20472 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1189,6 +1189,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
> > > * get lost on g4x as well, and interrupt delivery seems to stay
> > > * properly dead afterwards. So we'll just disable them for all
> > > * pre-gen5 chipsets.
> > > + *
> > > + * dp aux and gmbus irq on gen4 seems to be able to generate legacy
> > > + * interrupts even when in MSI mode. This results in spurious
> > > + * interrupt warnings if the legacy irq no. is shared with another
> > > + * device. The kernel then disables that interrupt source and so
> > > + * prevents the other device from working properly.
> > > */
> > > if (INTEL_GEN(dev_priv) >= 5) {
> > > if (pci_enable_msi(pdev) < 0)
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index b86ed6401120..c5e1f648c47c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2558,16 +2558,6 @@ intel_info(const struct drm_i915_private *dev_priv)
> > > (IS_CANNONLAKE(dev_priv) || \
> > > IS_SKL_GT3(dev_priv) || IS_SKL_GT4(dev_priv))
> > >
> > > -/*
> > > - * dp aux and gmbus irq on gen4 seems to be able to generate legacy interrupts
> > > - * even when in MSI mode. This results in spurious interrupt warnings if the
> > > - * legacy irq no. is shared with another device. The kernel then disables that
> > > - * interrupt source and so prevents the other device from working properly.
> > > - *
> > > - * Since we don't enable MSI anymore on gen4, we can always use GMBUS/AUX
> > > - * interrupts.
> > > - */
> > > -#define HAS_AUX_IRQ(dev_priv) true
> > > #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
> > >
> > > /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index ce07bd794aed..1dab40056df7 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -936,7 +936,7 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
> > > }
> > >
> > > static uint32_t
> > > -intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> > > +intel_dp_aux_wait_done(struct intel_dp *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> > > i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > > @@ -944,14 +944,10 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> > > bool done;
> > >
> > > #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > > - if (has_aux_irq)
> > > - done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> > > - msecs_to_jiffies_timeout(10));
> > > - else
> > > - done = wait_for(C, 10) == 0;
> > > + done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> > > + msecs_to_jiffies_timeout(10));
> > > if (!done)
> > > - DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
> > > - has_aux_irq);
> > > + DRM_ERROR("dp aux hw did not signal timeout!\n");
> > > #undef C
> > >
> > > return status;
> > > @@ -1016,7 +1012,6 @@ static uint32_t skl_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> > > }
> > >
> > > static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > - bool has_aux_irq,
> > > int send_bytes,
> > > uint32_t aux_clock_divider)
> > > {
> > > @@ -1037,7 +1032,7 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> > >
> > > return DP_AUX_CH_CTL_SEND_BUSY |
> > > DP_AUX_CH_CTL_DONE |
> > > - (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> > > + DP_AUX_CH_CTL_INTERRUPT |
> > > DP_AUX_CH_CTL_TIME_OUT_ERROR |
> > > timeout |
> > > DP_AUX_CH_CTL_RECEIVE_ERROR |
> > > @@ -1047,13 +1042,12 @@ static uint32_t g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > }
> > >
> > > static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp,
> > > - bool has_aux_irq,
> > > int send_bytes,
> > > uint32_t unused)
> > > {
> > > return DP_AUX_CH_CTL_SEND_BUSY |
> > > DP_AUX_CH_CTL_DONE |
> > > - (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
> > > + DP_AUX_CH_CTL_INTERRUPT |
> > > DP_AUX_CH_CTL_TIME_OUT_ERROR |
> > > DP_AUX_CH_CTL_TIME_OUT_MAX |
> > > DP_AUX_CH_CTL_RECEIVE_ERROR |
> > > @@ -1076,7 +1070,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > > int i, ret, recv_bytes;
> > > uint32_t status;
> > > int try, clock = 0;
> > > - bool has_aux_irq = HAS_AUX_IRQ(dev_priv);
> > > bool vdd;
> > >
> > > ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
> > > @@ -1131,7 +1124,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > >
> > > while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> > > u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> > > - has_aux_irq,
> > > send_bytes,
> > > aux_clock_divider);
> > >
> > > @@ -1148,7 +1140,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > > /* Send the command and wait for it to complete */
> > > I915_WRITE(ch_ctl, send_ctl);
> > >
> > > - status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
> > > + status = intel_dp_aux_wait_done(intel_dp);
> > >
> > > /* Clear done status and any errors */
> > > I915_WRITE(ch_ctl,
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 2855363f5b6c..7ac91d5ee3fa 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1138,7 +1138,6 @@ struct intel_dp {
> > > * register with to kick off an AUX transaction.
> > > */
> > > uint32_t (*get_aux_send_ctl)(struct intel_dp *dp,
> > > - bool has_aux_irq,
> > > int send_bytes,
> > > uint32_t aux_clock_divider);
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index db27f2faa1de..ee26f5f33b87 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -373,7 +373,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
> > > aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> > >
> > > /* Start with bits set for DDI_AUX_CTL register */
> > > - aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, 0, sizeof(aux_msg),
> > > + aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
> > > aux_clock_divider);
> >
> > Hmm. What's the situation with the PSR stuff here?
>
> I hadn't notice this, thanks.
>
> I don't see why it would actually be correct though. It seems to be a
> bug since d4dcbdceabe4 ("drm/i915/psr: Use ->get_aux_send_ctl
> functions") when the rmw was removed (so whatever was on the register
> before get overwritten).
Dunno why you think there's a bug. The problem I see with this patch is
that you're now setting the irq enable bit on the psr aux register.
Does that bit even exist? If so does it do what we want it to do?
>
> CC'ing Daniel who wrote that patch and José/DK who worked recently on
> psr stuff.
>
>
> Lucas De Marchi
> >
> > >
> > > /* Select only valid bits for SRD_AUX_CTL */
> > > --
> > > 2.17.0
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
