| Message ID | 20180403212420.25007-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-04-03 at 14:24 -0700, Dhinakaran Pandiyan wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > The definitions for the error register should be valid on bdw/skl > too, > but there we haven't even enabled DE_MISC handling yet. > > Somewhat confusing the the moved register offset on bdw is only for > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been > on bdw. > > v2: Fixes from Ville. > > v3: From DK > * Rebased on drm-tip > * Removed BDW IIR bit definition, looks like an unintentional change > that > should be in the following patch. > > v4: From DK > * Don't mask REG_WRITE. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> With bspec id and comment about why are you masking interruptions in hsw_edp_psr_irq_handler() feel free to add: Reviewed-by: Jose Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 34 > ++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > b/drivers/gpu/drm/i915/i915_irq.c > index 27aee25429b7..c2d3f30778ee 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2391,6 +2391,26 @@ static void ilk_display_irq_handler(struct > drm_i915_private *dev_priv, > ironlake_rps_change_irq_handler(dev_priv); > } > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private > *dev_priv) > +{ > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR); > + > + if (edp_psr_iir & EDP_PSR_ERROR) > + DRM_DEBUG_KMS("PSR error\n"); > + > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) { > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n"); > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY); Just to know... you need to mask this one otherwise it will keep triggering EDP_PSR_PRE_ENTRY interruptions? Would be nice to have a comment explaning why you are masking it. > + } > + > + if (edp_psr_iir & EDP_PSR_POST_EXIT) { > + DRM_DEBUG_KMS("PSR exit completed\n"); > + I915_WRITE(EDP_PSR_IMR, 0); > + } > + > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir); > +} > + > static void ivb_display_irq_handler(struct drm_i915_private > *dev_priv, > u32 de_iir) > { > @@ -2403,6 +2423,9 @@ static void ivb_display_irq_handler(struct > drm_i915_private *dev_priv, > if (de_iir & DE_ERR_INT_IVB) > ivb_err_int_handler(dev_priv); > > + if (de_iir & DE_EDP_PSR_INT_HSW) > + hsw_edp_psr_irq_handler(dev_priv); > + > if (de_iir & DE_AUX_CHANNEL_A_IVB) > dp_aux_irq_handler(dev_priv); > > @@ -3260,6 +3283,11 @@ static void ironlake_irq_reset(struct > drm_device *dev) > if (IS_GEN7(dev_priv)) > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > > + if (IS_HASWELL(dev_priv)) { > + I915_WRITE(EDP_PSR_IMR, 0xffffffff); > + I915_WRITE(EDP_PSR_IIR, 0xffffffff); No need to do a POSTING_READ(EDP_PSR_IMR);? Like is done in: GEN3_IRQ_RESET() > + } > + > gen5_gt_irq_reset(dev_priv); > > ibx_irq_reset(dev_priv); > @@ -3671,6 +3699,12 @@ static int ironlake_irq_postinstall(struct > drm_device *dev) > DE_DP_A_HOTPLUG); > } > > + if (IS_HASWELL(dev_priv)) { > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR); > + I915_WRITE(EDP_PSR_IMR, 0); > + display_mask |= DE_EDP_PSR_INT_HSW; > + } > + > dev_priv->irq_mask = ~display_mask; > > ibx_irq_pre_postinstall(dev); > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > index 176dca6554f4..f5783d6db614 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4011,6 +4011,13 @@ enum { > #define EDP_PSR_TP1_TIME_0us (3<<4) > #define EDP_PSR_IDLE_FRAME_SHIFT 0 > > +/* Bspec claims those aren't shifted but stay at 0x64800 */ > +#define EDP_PSR_IMR _MMIO(0x64834) > +#define EDP_PSR_IIR _MMIO(0x64838) > +#define EDP_PSR_ERROR (1<<2) > +#define EDP_PSR_POST_EXIT (1<<1) > +#define EDP_PSR_PRE_ENTRY (1<<0) Could you add the bspec id of where did you got this? The hsw spec that I'm reading don't have the bits, skl have but don't have the bits of the other transcoders. > + > #define EDP_PSR_AUX_CTL _MMIO(dev_pri > v->psr_mmio_base + 0x10) > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > #define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK (0x1f << 20) > @@ -6820,6 +6827,7 @@ enum { > #define DE_PCH_EVENT_IVB (1<<28) > #define DE_DP_A_HOTPLUG_IVB (1<<27) > #define DE_AUX_CHANNEL_A_IVB (1<<26) > +#define DE_EDP_PSR_INT_HSW (1<<19) > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14) > #define DE_PLANEC_FLIP_DONE_IVB (1<<13) > #define DE_PIPEC_VBLANK_IVB (1<<10)
On Thu, 2018-04-05 at 20:40 +0000, Souza, Jose wrote: > On Tue, 2018-04-03 at 14:24 -0700, Dhinakaran Pandiyan wrote: > > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > The definitions for the error register should be valid on bdw/skl > > too, > > but there we haven't even enabled DE_MISC handling yet. > > > > Somewhat confusing the the moved register offset on bdw is only for > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have been > > on bdw. > > > > v2: Fixes from Ville. > > > > v3: From DK > > * Rebased on drm-tip > > * Removed BDW IIR bit definition, looks like an unintentional change > > that > > should be in the following patch. > > > > v4: From DK > > * Don't mask REG_WRITE. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > With bspec id and comment about why are you masking interruptions in > hsw_edp_psr_irq_handler() feel free to add: > > Reviewed-by: Jose Roberto de Souza <jose.souza@intel.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 34 > > ++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ > > 2 files changed, 42 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 27aee25429b7..c2d3f30778ee 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2391,6 +2391,26 @@ static void ilk_display_irq_handler(struct > > drm_i915_private *dev_priv, > > ironlake_rps_change_irq_handler(dev_priv); > > } > > > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private > > *dev_priv) > > +{ > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR); > > + > > + if (edp_psr_iir & EDP_PSR_ERROR) > > + DRM_DEBUG_KMS("PSR error\n"); > > + > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) { > > + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n"); > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY); > > Just to know... you need to mask this one otherwise it will keep > triggering EDP_PSR_PRE_ENTRY interruptions? Would be nice to have a > comment explaning why you are masking it. > The final implementation in patch 3/4 doesn't do that. Adding a comment here and removing will be pointless IMO. > > + } > > + > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) { > > + DRM_DEBUG_KMS("PSR exit completed\n"); > > + I915_WRITE(EDP_PSR_IMR, 0); > > + } > > + > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir); > > +} > > + > > static void ivb_display_irq_handler(struct drm_i915_private > > *dev_priv, > > u32 de_iir) > > { > > @@ -2403,6 +2423,9 @@ static void ivb_display_irq_handler(struct > > drm_i915_private *dev_priv, > > if (de_iir & DE_ERR_INT_IVB) > > ivb_err_int_handler(dev_priv); > > > > + if (de_iir & DE_EDP_PSR_INT_HSW) > > + hsw_edp_psr_irq_handler(dev_priv); > > + > > if (de_iir & DE_AUX_CHANNEL_A_IVB) > > dp_aux_irq_handler(dev_priv); > > > > @@ -3260,6 +3283,11 @@ static void ironlake_irq_reset(struct > > drm_device *dev) > > if (IS_GEN7(dev_priv)) > > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > > > > + if (IS_HASWELL(dev_priv)) { > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff); > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff); > > No need to do a POSTING_READ(EDP_PSR_IMR);? Like is done in: > GEN3_IRQ_RESET() > We should be fine without that. From what I was told a while ago, some of these POSTING_READS are cargo culted. > > + } > > + > > gen5_gt_irq_reset(dev_priv); > > > > ibx_irq_reset(dev_priv); > > @@ -3671,6 +3699,12 @@ static int ironlake_irq_postinstall(struct > > drm_device *dev) > > DE_DP_A_HOTPLUG); > > } > > > > + if (IS_HASWELL(dev_priv)) { > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR); > > + I915_WRITE(EDP_PSR_IMR, 0); > > + display_mask |= DE_EDP_PSR_INT_HSW; > > + } > > + > > dev_priv->irq_mask = ~display_mask; > > > > ibx_irq_pre_postinstall(dev); > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 176dca6554f4..f5783d6db614 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4011,6 +4011,13 @@ enum { > > #define EDP_PSR_TP1_TIME_0us (3<<4) > > #define EDP_PSR_IDLE_FRAME_SHIFT 0 > > > > +/* Bspec claims those aren't shifted but stay at 0x64800 */ > > +#define EDP_PSR_IMR _MMIO(0x64834) > > +#define EDP_PSR_IIR _MMIO(0x64838) > > +#define EDP_PSR_ERROR (1<<2) > > +#define EDP_PSR_POST_EXIT (1<<1) > > +#define EDP_PSR_PRE_ENTRY (1<<0) > > Could you add the bspec id of where did you got this? > The hsw spec that I'm reading don't have the bits, skl have but don't > have the bits of the other transcoders. > I understand it is not easy to find, but are we going to add bspec references for all new register definitions? :) From what I have seen, a bspec reference is typically added only for workarounds. I'll update this patch though since I've waited too long to get this patch in. Would adding the bspec reference in the commit message suffice? > > + > > #define EDP_PSR_AUX_CTL _MMIO(dev_pri > > v->psr_mmio_base + 0x10) > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > #define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK (0x1f << 20) > > @@ -6820,6 +6827,7 @@ enum { > > #define DE_PCH_EVENT_IVB (1<<28) > > #define DE_DP_A_HOTPLUG_IVB (1<<27) > > #define DE_AUX_CHANNEL_A_IVB (1<<26) > > +#define DE_EDP_PSR_INT_HSW (1<<19) > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14) > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13) > > #define DE_PIPEC_VBLANK_IVB (1<<10)
On Thu, 2018-04-05 at 14:42 -0700, Dhinakaran Pandiyan wrote: > > > On Thu, 2018-04-05 at 20:40 +0000, Souza, Jose wrote: > > On Tue, 2018-04-03 at 14:24 -0700, Dhinakaran Pandiyan wrote: > > > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > The definitions for the error register should be valid on bdw/skl > > > too, > > > but there we haven't even enabled DE_MISC handling yet. > > > > > > Somewhat confusing the the moved register offset on bdw is only > > > for > > > the _CTL/_AUX register, and that _IIR/IMR stayed where they have > > > been > > > on bdw. > > > > > > v2: Fixes from Ville. > > > > > > v3: From DK > > > * Rebased on drm-tip > > > * Removed BDW IIR bit definition, looks like an unintentional > > > change > > > that > > > should be in the following patch. > > > > > > v4: From DK > > > * Don't mask REG_WRITE. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > With bspec id and comment about why are you masking interruptions > > in > > hsw_edp_psr_irq_handler() feel free to add: > > > > Reviewed-by: Jose Roberto de Souza <jose.souza@intel.com> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 34 > > > ++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ > > > 2 files changed, 42 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > b/drivers/gpu/drm/i915/i915_irq.c > > > index 27aee25429b7..c2d3f30778ee 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -2391,6 +2391,26 @@ static void ilk_display_irq_handler(struct > > > drm_i915_private *dev_priv, > > > ironlake_rps_change_irq_handler(dev_priv); > > > } > > > > > > +static void hsw_edp_psr_irq_handler(struct drm_i915_private > > > *dev_priv) > > > +{ > > > + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR); > > > + > > > + if (edp_psr_iir & EDP_PSR_ERROR) > > > + DRM_DEBUG_KMS("PSR error\n"); > > > + > > > + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) { > > > + DRM_DEBUG_KMS("PSR prepare entry in 2 > > > vblanks\n"); > > > + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY); > > > > Just to know... you need to mask this one otherwise it will keep > > triggering EDP_PSR_PRE_ENTRY interruptions? Would be nice to have a > > comment explaning why you are masking it. > > > > The final implementation in patch 3/4 doesn't do that. Adding a > comment > here and removing will be pointless IMO. Okay > > > > + } > > > + > > > + if (edp_psr_iir & EDP_PSR_POST_EXIT) { > > > + DRM_DEBUG_KMS("PSR exit completed\n"); > > > + I915_WRITE(EDP_PSR_IMR, 0); > > > + } > > > + > > > + I915_WRITE(EDP_PSR_IIR, edp_psr_iir); > > > +} > > > + > > > static void ivb_display_irq_handler(struct drm_i915_private > > > *dev_priv, > > > u32 de_iir) > > > { > > > @@ -2403,6 +2423,9 @@ static void ivb_display_irq_handler(struct > > > drm_i915_private *dev_priv, > > > if (de_iir & DE_ERR_INT_IVB) > > > ivb_err_int_handler(dev_priv); > > > > > > + if (de_iir & DE_EDP_PSR_INT_HSW) > > > + hsw_edp_psr_irq_handler(dev_priv); > > > + > > > if (de_iir & DE_AUX_CHANNEL_A_IVB) > > > dp_aux_irq_handler(dev_priv); > > > > > > @@ -3260,6 +3283,11 @@ static void ironlake_irq_reset(struct > > > drm_device *dev) > > > if (IS_GEN7(dev_priv)) > > > I915_WRITE(GEN7_ERR_INT, 0xffffffff); > > > > > > + if (IS_HASWELL(dev_priv)) { > > > + I915_WRITE(EDP_PSR_IMR, 0xffffffff); > > > + I915_WRITE(EDP_PSR_IIR, 0xffffffff); > > > > No need to do a POSTING_READ(EDP_PSR_IMR);? Like is done in: > > GEN3_IRQ_RESET() > > > > We should be fine without that. From what I was told a while ago, > some > of these POSTING_READS are cargo culted. Ack > > > > + } > > > + > > > gen5_gt_irq_reset(dev_priv); > > > > > > ibx_irq_reset(dev_priv); > > > @@ -3671,6 +3699,12 @@ static int ironlake_irq_postinstall(struct > > > drm_device *dev) > > > DE_DP_A_HOTPLUG); > > > } > > > > > > + if (IS_HASWELL(dev_priv)) { > > > + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR); > > > + I915_WRITE(EDP_PSR_IMR, 0); > > > + display_mask |= DE_EDP_PSR_INT_HSW; > > > + } > > > + > > > dev_priv->irq_mask = ~display_mask; > > > > > > ibx_irq_pre_postinstall(dev); > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > > b/drivers/gpu/drm/i915/i915_reg.h > > > index 176dca6554f4..f5783d6db614 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -4011,6 +4011,13 @@ enum { > > > #define EDP_PSR_TP1_TIME_0us (3<<4) > > > #define EDP_PSR_IDLE_FRAME_SHIFT 0 > > > > > > +/* Bspec claims those aren't shifted but stay at 0x64800 */ > > > +#define EDP_PSR_IMR _MMIO(0x64834 > > > ) > > > +#define EDP_PSR_IIR _MMIO(0x64838 > > > ) > > > +#define EDP_PSR_ERROR (1<<2) > > > +#define EDP_PSR_POST_EXIT (1<<1) > > > +#define EDP_PSR_PRE_ENTRY (1<<0) > > > > Could you add the bspec id of where did you got this? > > The hsw spec that I'm reading don't have the bits, skl have but > > don't > > have the bits of the other transcoders. > > > > I understand it is not easy to find, but are we going to add bspec > references for all new register definitions? :) From what I have > seen, a > bspec reference is typically added only for workarounds. I'll update > this patch though since I've waited too long to get this patch in. > Would > adding the bspec reference in the commit message suffice? Yeah in the commit message please. > > > > > + > > > #define EDP_PSR_AUX_CTL _MMIO(dev > > > _pri > > > v->psr_mmio_base + 0x10) > > > #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) > > > #define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK (0x1f << 20) > > > @@ -6820,6 +6827,7 @@ enum { > > > #define DE_PCH_EVENT_IVB (1<<28) > > > #define DE_DP_A_HOTPLUG_IVB (1<<27) > > > #define DE_AUX_CHANNEL_A_IVB (1<<26) > > > +#define DE_EDP_PSR_INT_HSW (1<<19) > > > #define DE_SPRITEC_FLIP_DONE_IVB (1<<14) > > > #define DE_PLANEC_FLIP_DONE_IVB (1<<13) > > > #define DE_PIPEC_VBLANK_IVB (1<<10) > >
On Tue, Apr 10, 2018 at 12:49:25AM -0000, Patchwork wrote: > == Series Details == > > Series: series starting with [v4] drm/i915: Enable edp psr error interrupts on hsw (rev3) > URL : https://patchwork.freedesktop.org/series/41095/ > State : warning > > == Summary == > > $ dim checkpatch origin/drm-tip > 0a22dbbae8f8 drm/i915: Enable edp psr error interrupts on hsw > -:111: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #111: FILE: drivers/gpu/drm/i915/i915_reg.h:4032: > +#define EDP_PSR_ERROR (1<<2) > ^ > > -:112: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #112: FILE: drivers/gpu/drm/i915/i915_reg.h:4033: > +#define EDP_PSR_POST_EXIT (1<<1) > ^ > > -:113: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #113: FILE: drivers/gpu/drm/i915/i915_reg.h:4034: > +#define EDP_PSR_PRE_ENTRY (1<<0) > ^ > > -:122: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > #122: FILE: drivers/gpu/drm/i915/i915_reg.h:6847: > +#define DE_EDP_PSR_INT_HSW (1<<19) > ^ > > total: 0 errors, 0 warnings, 4 checks, 78 lines checked > 7fdf2eed9ed4 drm/i915: Enable edp psr error interrupts on bdw+ > -:159: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses > #159: FILE: drivers/gpu/drm/i915/intel_display.h:221: > +#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \ > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > + for_each_if ((__mask) & (1 << (__t))) This showed up on red on dim when I was going to push here... DK, could you please address this one here before we can push? Thanks, Rodrigo. > > -:159: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__t' - possible side-effects? > #159: FILE: drivers/gpu/drm/i915/intel_display.h:221: > +#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \ > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > + for_each_if ((__mask) & (1 << (__t))) > > -:160: CHECK:SPACING: No space is necessary after a cast > #160: FILE: drivers/gpu/drm/i915/intel_display.h:222: > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > -:161: WARNING:SPACING: space prohibited between function name and open parenthesis '(' > #161: FILE: drivers/gpu/drm/i915/intel_display.h:223: > + for_each_if ((__mask) & (1 << (__t))) > > total: 1 errors, 1 warnings, 2 checks, 123 lines checked > 003ec0005027 drm/i915/psr: Control PSR interrupts via debugfs > d8186b823b62 drm/i915/psr: Timestamps for PSR entry and exit interrupts. > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2018-04-10 at 10:59 -0700, Rodrigo Vivi wrote: > On Tue, Apr 10, 2018 at 12:49:25AM -0000, Patchwork wrote: > > == Series Details == > > > > Series: series starting with [v4] drm/i915: Enable edp psr error interrupts on hsw (rev3) > > URL : https://patchwork.freedesktop.org/series/41095/ > > State : warning > > > > == Summary == > > > > $ dim checkpatch origin/drm-tip > > 0a22dbbae8f8 drm/i915: Enable edp psr error interrupts on hsw > > -:111: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > #111: FILE: drivers/gpu/drm/i915/i915_reg.h:4032: > > +#define EDP_PSR_ERROR (1<<2) > > ^ > > > > -:112: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > #112: FILE: drivers/gpu/drm/i915/i915_reg.h:4033: > > +#define EDP_PSR_POST_EXIT (1<<1) > > ^ > > > > -:113: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > #113: FILE: drivers/gpu/drm/i915/i915_reg.h:4034: > > +#define EDP_PSR_PRE_ENTRY (1<<0) > > ^ > > > > -:122: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > #122: FILE: drivers/gpu/drm/i915/i915_reg.h:6847: > > +#define DE_EDP_PSR_INT_HSW (1<<19) > > ^ > > > > total: 0 errors, 0 warnings, 4 checks, 78 lines checked > > 7fdf2eed9ed4 drm/i915: Enable edp psr error interrupts on bdw+ > > -:159: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses > > #159: FILE: drivers/gpu/drm/i915/intel_display.h:221: > > +#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \ > > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > + for_each_if ((__mask) & (1 << (__t))) > > This showed up on red on dim when I was going to push here... > > DK, could you please address this one here before we can push? > The macros look correct to me, that is how other macros are written too. check_patch is confused? > Thanks, > Rodrigo. > > > > > -:159: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__t' - possible side-effects? > > #159: FILE: drivers/gpu/drm/i915/intel_display.h:221: > > +#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \ > > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > + for_each_if ((__mask) & (1 << (__t))) > > > > -:160: CHECK:SPACING: No space is necessary after a cast > > #160: FILE: drivers/gpu/drm/i915/intel_display.h:222: > > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > > > -:161: WARNING:SPACING: space prohibited between function name and open parenthesis '(' > > #161: FILE: drivers/gpu/drm/i915/intel_display.h:223: > > + for_each_if ((__mask) & (1 << (__t))) > > > > total: 1 errors, 1 warnings, 2 checks, 123 lines checked > > 003ec0005027 drm/i915/psr: Control PSR interrupts via debugfs > > d8186b823b62 drm/i915/psr: Timestamps for PSR entry and exit interrupts. > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Apr 10, 2018 at 11:29:09AM -0700, Dhinakaran Pandiyan wrote: > > > > On Tue, 2018-04-10 at 10:59 -0700, Rodrigo Vivi wrote: > > On Tue, Apr 10, 2018 at 12:49:25AM -0000, Patchwork wrote: > > > == Series Details == > > > > > > Series: series starting with [v4] drm/i915: Enable edp psr error interrupts on hsw (rev3) > > > URL : https://patchwork.freedesktop.org/series/41095/ > > > State : warning > > > > > > == Summary == > > > > > > $ dim checkpatch origin/drm-tip > > > 0a22dbbae8f8 drm/i915: Enable edp psr error interrupts on hsw > > > -:111: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > > #111: FILE: drivers/gpu/drm/i915/i915_reg.h:4032: > > > +#define EDP_PSR_ERROR (1<<2) > > > ^ > > > > > > -:112: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > > #112: FILE: drivers/gpu/drm/i915/i915_reg.h:4033: > > > +#define EDP_PSR_POST_EXIT (1<<1) > > > ^ > > > > > > -:113: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > > #113: FILE: drivers/gpu/drm/i915/i915_reg.h:4034: > > > +#define EDP_PSR_PRE_ENTRY (1<<0) > > > ^ > > > > > > -:122: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV) > > > #122: FILE: drivers/gpu/drm/i915/i915_reg.h:6847: > > > +#define DE_EDP_PSR_INT_HSW (1<<19) > > > ^ > > > > > > total: 0 errors, 0 warnings, 4 checks, 78 lines checked > > > 7fdf2eed9ed4 drm/i915: Enable edp psr error interrupts on bdw+ > > > -:159: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses > > > #159: FILE: drivers/gpu/drm/i915/intel_display.h:221: > > > +#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \ > > > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > > + for_each_if ((__mask) & (1 << (__t))) > > > > This showed up on red on dim when I was going to push here... > > > > DK, could you please address this one here before we can push? > > > > The macros look correct to me, that is how other macros are written too. > check_patch is confused? Well, you are right. New macro is just like the other for_each_* If we need a clean we can do that later, or silent dim on that... Anyways, pushed this patches to dinq. Thanks for patches, reviews, patience and extra checks ;) > > > Thanks, > > Rodrigo. > > > > > > > > -:159: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__t' - possible side-effects? > > > #159: FILE: drivers/gpu/drm/i915/intel_display.h:221: > > > +#define for_each_cpu_transcoder_masked(__dev_priv, __t, __mask) \ > > > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > > + for_each_if ((__mask) & (1 << (__t))) > > > > > > -:160: CHECK:SPACING: No space is necessary after a cast > > > #160: FILE: drivers/gpu/drm/i915/intel_display.h:222: > > > + for ((__t) = 0; (__t) < I915_MAX_TRANSCODERS; (__t)++) \ > > > > > > -:161: WARNING:SPACING: space prohibited between function name and open parenthesis '(' > > > #161: FILE: drivers/gpu/drm/i915/intel_display.h:223: > > > + for_each_if ((__mask) & (1 << (__t))) > > > > > > total: 1 errors, 1 warnings, 2 checks, 123 lines checked > > > 003ec0005027 drm/i915/psr: Control PSR interrupts via debugfs > > > d8186b823b62 drm/i915/psr: Timestamps for PSR entry and exit interrupts. > > > > > > _______________________________________________ > > > 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_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 27aee25429b7..c2d3f30778ee 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2391,6 +2391,26 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv, ironlake_rps_change_irq_handler(dev_priv); } +static void hsw_edp_psr_irq_handler(struct drm_i915_private *dev_priv) +{ + u32 edp_psr_iir = I915_READ(EDP_PSR_IIR); + + if (edp_psr_iir & EDP_PSR_ERROR) + DRM_DEBUG_KMS("PSR error\n"); + + if (edp_psr_iir & EDP_PSR_PRE_ENTRY) { + DRM_DEBUG_KMS("PSR prepare entry in 2 vblanks\n"); + I915_WRITE(EDP_PSR_IMR, EDP_PSR_PRE_ENTRY); + } + + if (edp_psr_iir & EDP_PSR_POST_EXIT) { + DRM_DEBUG_KMS("PSR exit completed\n"); + I915_WRITE(EDP_PSR_IMR, 0); + } + + I915_WRITE(EDP_PSR_IIR, edp_psr_iir); +} + static void ivb_display_irq_handler(struct drm_i915_private *dev_priv, u32 de_iir) { @@ -2403,6 +2423,9 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv, if (de_iir & DE_ERR_INT_IVB) ivb_err_int_handler(dev_priv); + if (de_iir & DE_EDP_PSR_INT_HSW) + hsw_edp_psr_irq_handler(dev_priv); + if (de_iir & DE_AUX_CHANNEL_A_IVB) dp_aux_irq_handler(dev_priv); @@ -3260,6 +3283,11 @@ static void ironlake_irq_reset(struct drm_device *dev) if (IS_GEN7(dev_priv)) I915_WRITE(GEN7_ERR_INT, 0xffffffff); + if (IS_HASWELL(dev_priv)) { + I915_WRITE(EDP_PSR_IMR, 0xffffffff); + I915_WRITE(EDP_PSR_IIR, 0xffffffff); + } + gen5_gt_irq_reset(dev_priv); ibx_irq_reset(dev_priv); @@ -3671,6 +3699,12 @@ static int ironlake_irq_postinstall(struct drm_device *dev) DE_DP_A_HOTPLUG); } + if (IS_HASWELL(dev_priv)) { + gen3_assert_iir_is_zero(dev_priv, EDP_PSR_IIR); + I915_WRITE(EDP_PSR_IMR, 0); + display_mask |= DE_EDP_PSR_INT_HSW; + } + dev_priv->irq_mask = ~display_mask; ibx_irq_pre_postinstall(dev); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 176dca6554f4..f5783d6db614 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4011,6 +4011,13 @@ enum { #define EDP_PSR_TP1_TIME_0us (3<<4) #define EDP_PSR_IDLE_FRAME_SHIFT 0 +/* Bspec claims those aren't shifted but stay at 0x64800 */ +#define EDP_PSR_IMR _MMIO(0x64834) +#define EDP_PSR_IIR _MMIO(0x64838) +#define EDP_PSR_ERROR (1<<2) +#define EDP_PSR_POST_EXIT (1<<1) +#define EDP_PSR_PRE_ENTRY (1<<0) + #define EDP_PSR_AUX_CTL _MMIO(dev_priv->psr_mmio_base + 0x10) #define EDP_PSR_AUX_CTL_TIME_OUT_MASK (3 << 26) #define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK (0x1f << 20) @@ -6820,6 +6827,7 @@ enum { #define DE_PCH_EVENT_IVB (1<<28) #define DE_DP_A_HOTPLUG_IVB (1<<27) #define DE_AUX_CHANNEL_A_IVB (1<<26) +#define DE_EDP_PSR_INT_HSW (1<<19) #define DE_SPRITEC_FLIP_DONE_IVB (1<<14) #define DE_PLANEC_FLIP_DONE_IVB (1<<13) #define DE_PIPEC_VBLANK_IVB (1<<10)