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@xxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> > 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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
