| Message ID | 20180523180435.18042-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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_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? > > /* Select only valid bits for SRD_AUX_CTL */ > -- > 2.17.0
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_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). 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
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_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
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_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? 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() 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
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. > > 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 > >
On Tue, Jun 19, 2018 at 08:24:33AM -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_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? > > 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() OK. Seems to all check out. Pushed to dinq. Thanks for the patch.
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 > > > >
On Thu, 2018-06-21 at 10:04 -0700, Lucas De Marchi wrote: > 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_time > > > > > > > out( > > > > > > > 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_a > > > > > > > ux_i > > > > > > > rq, > > > > > > > send_ > > > > > > > byte > > > > > > > s, > > > > > > > aux_c > > > > > > > lock > > > > > > > _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. I see that psr_irq_handler() gets called when the PSR bit is set in DEIIR/GEN8_DE_MISC_IIR, although we don't do much after the interrupt is handled. Do you mean something else? > > > 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
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); /* Select only valid bits for SRD_AUX_CTL */
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(-)