All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lucas De Marchi <lucas.de.marchi@gmail.com>
To: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org,
	Lucas De Marchi <lucas.demarchi@intel.com>
Subject: Re: [PATCH v2] drm/i915: remove check for aux irq
Date: Thu, 21 Jun 2018 10:04:55 -0700	[thread overview]
Message-ID: <20180621170453.GB6955@ldmartin-desk.amr.corp.intel.com> (raw)
In-Reply-To: <1529439430.12516.8.camel@intel.com>

On Tue, Jun 19, 2018 at 01:17:10PM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-19 at 08:24 -0700, Lucas De Marchi wrote:
> > On Tue, Jun 19, 2018 at 7:06 AM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > 
> > > 
> > > 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@linux.intel.com>
> > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > > ---
> > > > > >  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_i
> > > > > > rq,
> > > > > >                                                     send_byte
> > > > > > s,
> > > > > >                                                     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?
> > I was assuming they should be the same. Now I see the psr code path
> > gets the bits for
> > DDI_AUX_CTL and mask to use only the valid bits for EDP_PSR_AUX_CTL.
> > Bit 28 will
> > be left out regardless the return of get_aux_send_ctl()
> Right.
> 
> Looking at this code, I realized we should enable the error interrupt
> (bit-11) for PSR_AUX_CTL too.

You mean enable it and handle the interrupt, right? Because it's
currently not being handled.


Lucas De Marchi


> 
> 
> > 
> > Lucas De Marchi
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2018-06-21 17:04 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25 21:55 [PATCH] drm/i915: remove check for aux irq Lucas De Marchi
2018-04-25 22:11 ` ✗ Fi.CI.SPARSE: warning for " Patchwork
2018-04-25 22:28 ` ✓ Fi.CI.BAT: success " Patchwork
2018-04-25 23:24 ` [PATCH] " Rodrigo Vivi
2018-04-26  2:45 ` ✓ Fi.CI.IGT: success for " Patchwork
2018-04-26 13:43 ` [PATCH] " Ville Syrjälä
2018-04-26 15:22   ` Lucas De Marchi
2018-04-26 15:27     ` Ville Syrjälä
2018-04-26 15:42       ` Lucas De Marchi
2018-04-26 15:50         ` Ville Syrjälä
2018-04-30 22:05           ` Lucas De Marchi
2018-05-02 12:07             ` Ville Syrjälä
2018-05-23 18:04               ` [PATCH v2] " Lucas De Marchi
2018-06-15 17:58                 ` Ville Syrjälä
2018-06-15 21:51                   ` Lucas De Marchi
2018-06-19 14:02                     ` Ville Syrjälä
2018-06-19 15:24                       ` Lucas De Marchi
2018-06-19 20:17                         ` Dhinakaran Pandiyan
2018-06-21 17:04                           ` Lucas De Marchi [this message]
2018-06-21 19:37                             ` Dhinakaran Pandiyan
2018-06-21 16:01                         ` Ville Syrjälä
2018-04-30 22:17 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: remove check for aux irq (rev2) Patchwork
2018-04-30 22:18 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-04-30 22:35 ` ✓ Fi.CI.BAT: success " Patchwork
2018-05-01  0:54 ` ✓ Fi.CI.IGT: " Patchwork
2018-05-23 18:13 ` ✗ Fi.CI.SPARSE: warning for drm/i915: remove check for aux irq (rev3) Patchwork
2018-05-23 18:28 ` ✓ Fi.CI.BAT: success " Patchwork
2018-05-23 22:38 ` ✓ Fi.CI.IGT: " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180621170453.GB6955@ldmartin-desk.amr.corp.intel.com \
    --to=lucas.de.marchi@gmail.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dhinakaran.pandiyan@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=lucas.demarchi@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.