[Intel-gfx] [RFC 1/7] drm/i915: Define flip done functions and enable IER

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni at intel.com>
> Sent: Tuesday, March 10, 2020 4:48 AM
> To: B S, Karthik <karthik.b.s at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: ville.syrjala at linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni at intel.com>; Shankar, Uma <uma.shankar at intel.com>
> 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 at intel.com>
> > ---
> >  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,