> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni@xxxxxxxxx>
> Sent: Tuesday, March 10, 2020 4:48 AM
> To: B S, Karthik <karthik.b.s@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: ville.syrjala@xxxxxxxxxxxxxxx; Kulkarni, Vandita
> <vandita.kulkarni@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>
> Subject: Re: [RFC 1/7] drm/i915: Define flip done functions and enable IER
>
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Add enable/disable flip done functions and enable the flip done
> > interrupt in IER.
> >
> > Flip done interrupt is used to send the page flip event as soon as the
> > surface address is written as per the requirement of async flips.
> >
> > Signed-off-by: Karthik B S <karthik.b.s@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 37
> > ++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_irq.h |
> > 2 ++
> > 2 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c index ecf07b0faad2..5955e737a45d
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2626,6 +2626,27 @@ int bdw_enable_vblank(struct drm_crtc *crtc)
> > return 0;
> > }
> >
> > +void icl_enable_flip_done(struct drm_crtc *crtc)
>
>
> Platform prefixes indicate the first platform that is able to run this function.
> In this case I can't even see which platforms will run the function because it's
> only later in the series that this function will get called. I'm not a fan of this
> patch splitting style where a function gets added in patch X and then used in
> patch X+Y. IMHO functions should only be introduced in patches where they
> are used.
> This makes the code much easier to review.
Thanks for the review.
Will update the patches as per your feedback.
>
> So, shouldn't this be skl_enable_flip_done()?
Agreed. Will update the function name.
>
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > + enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > + struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[pipe];
> > + unsigned long irqflags;
> > +
> > + /* Make sure that vblank is not enabled, as we are already sending
> > + * the page flip event in the flip_done_handler.
> > + */
> > + if (atomic_read(&vblank->refcount) != 0)
> > + drm_crtc_vblank_put(crtc);
>
> This is the kind of thing that will be much easier to review when this patch
> gets squashed in the one that makes use of these functions.
Will update the patches as per your feedback.
>
> Even after reading the whole series, this put() doesn't seem correct to me.
> What is the problem with having vblanks enabled? Is it because we were
> sending duplicate vblank events without these lines? Where is the
> get() that triggers this put()? Please help me understand this.
Checked the code once more after your review and I agree that this wouldn't be needed as
there is no get() called for which this put() would be needed.
And as the event is sent in the flip_done_handler in this series, there is no need to disable vblank.
>
>
> > +
> > + spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > + bdw_enable_pipe_irq(dev_priv, pipe,
> GEN9_PIPE_PLANE1_FLIP_DONE);
> > +
> > + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > +
> > +}
> > +
> > /* Called from drm generic code, passed 'crtc' which
> > * we use as a pipe index
> > */
> > @@ -2686,6 +2707,20 @@ void bdw_disable_vblank(struct drm_crtc *crtc)
> > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> >
> > +
> > +void icl_disable_flip_done(struct drm_crtc *crtc) {
> > + struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > + enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > + unsigned long irqflags;
> > +
> > + spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > + bdw_disable_pipe_irq(dev_priv, pipe,
> GEN9_PIPE_PLANE1_FLIP_DONE);
> > +
> > + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > +
> > static void ibx_irq_reset(struct drm_i915_private *dev_priv) {
> > struct intel_uncore *uncore = &dev_priv->uncore; @@ -3375,7
> +3410,7
> > @@ static void gen8_de_irq_postinstall(struct drm_i915_private
> *dev_priv)
> > de_port_masked |= CNL_AUX_CHANNEL_F;
> >
> > de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
> > - GEN8_PIPE_FIFO_UNDERRUN;
> > + GEN8_PIPE_FIFO_UNDERRUN |
> GEN9_PIPE_PLANE1_FLIP_DONE;
>
> This is going to set this bit for gen8 too, which is something we probably don't
> want since it doesn't exist there.
Will add a gen check here in the next revision.
>
> The patch also does not add the handler for the interrupt, which doesn't
> make sense (see my point above).
Noted.
>
> Also, don't we want to do like GEN8_PIPE_VBLANK and also set it on the
> power_well_post_enable hook? If not, why? This is probably a case we
> should write an IGT subtest for.
Will check this and update in the next revision.
>
> >
> > de_port_enables = de_port_masked;
> > if (IS_GEN9_LP(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/i915_irq.h
> > b/drivers/gpu/drm/i915/i915_irq.h index 812c47a9c2d6..6fc319980dd3
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.h
> > +++ b/drivers/gpu/drm/i915/i915_irq.h
> > @@ -114,11 +114,13 @@ int i915gm_enable_vblank(struct drm_crtc *crtc);
> > int i965_enable_vblank(struct drm_crtc *crtc); int
> > ilk_enable_vblank(struct drm_crtc *crtc); int
> > bdw_enable_vblank(struct drm_crtc *crtc);
> > +void icl_enable_flip_done(struct drm_crtc *crtc);
> > void i8xx_disable_vblank(struct drm_crtc *crtc); void
> > i915gm_disable_vblank(struct drm_crtc *crtc); void
> > i965_disable_vblank(struct drm_crtc *crtc); void
> > ilk_disable_vblank(struct drm_crtc *crtc); void
> > bdw_disable_vblank(struct drm_crtc *crtc);
> > +void icl_disable_flip_done(struct drm_crtc *crtc);
> >
> > void gen2_irq_reset(struct intel_uncore *uncore); void
> > gen3_irq_reset(struct intel_uncore *uncore, i915_reg_t imr,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
