Re: [PATCH 14/14] drm/i915: kerneldoc for intel_fifo_underrun.c

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 03, 2014 at 03:00:46PM -0300, Paulo Zanoni wrote:
> 2014-09-30 5:56 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Patch 10/14: Needs rebase, but ok.

Was just an || IS_GEN9 from the skl patches.

> Patch 11/14: Ok.
> Patch 12/14: Nice one, but:
> - I've seen you complaining about lines bigger than 80 columns before,
> so I obviously have to do the same here :)
> - Can't we now unexport (make them static)

Fixed.

> intel_set_{pch,cpu}_fifo_underrun_reporting()? Due to the rebasing
> problem I couldn't check this.

Still needed to hide underruns in some parts of the modeset sequence, so
gets called from intel_display.c. Kerneldoc even explains this ;-)
> Patch 13/14: Ok.
> Patch 14/14: Since you recently submitted a patch fixing spelling
> errors, I'll ask you to run a spell checker here too. I usually just
> go with ":set spell" in the only text editor that exists. Some stuff:
> PIPESTATE, occurance, interrup, interrup.

Fixed.

> For all patches, with or without changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks a lot for your review.

Cheers, Daniel
> 
> > ---
> >  Documentation/DocBook/drm.tmpl             |  5 ++
> >  drivers/gpu/drm/i915/intel_fifo_underrun.c | 82 +++++++++++++++++++++++-------
> >  2 files changed, 70 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index e8ef0f9c7396..9356f16847d9 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -3831,6 +3831,11 @@ int num_ioctls;</synopsis>
> >  !Fdrivers/gpu/drm/i915/i915_gem.c i915_gem_track_fb
> >        </sect2>
> >        <sect2>
> > +        <title>Display FIFO Underrun Reporting</title>
> > +!Pdrivers/gpu/drm/i915/intel_fifo_underrun.c fifo underrun handling
> > +!Idrivers/gpu/drm/i915/intel_fifo_underrun.c
> > +      </sect2>
> > +      <sect2>
> >          <title>Plane Configuration</title>
> >          <para>
> >           This section covers plane configuration and composition with the
> > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > index 02909259bfb6..dfffad41664e 100644
> > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > @@ -28,6 +28,26 @@
> >  #include "i915_drv.h"
> >  #include "intel_drv.h"
> >
> > +/**
> > + * DOC: fifo underrun handling
> > + *
> > + * The i915 driver checks for display fifo underruns using the interrupt signals
> > + * provided by the hardware. This is enabled by default and fairly useful to
> > + * debug display issues, especially watermark settings.
> > + *
> > + * If an underrun is detected this is logged into dmesg. To avoid flooding logs
> > + * and occupying the cpu underrun interrupts are disabled after the first
> > + * occurance until the next modeset on a given pipe.
> > + *
> > + * Note that underrun detection on gmch platforms is a bit more ugly since there
> > + * is no interrupt (despite that the signalling bit is in the PIPESTATE pipe
> > + * interrupt register). Also on some other platforms underrun interrupts are
> > + * shared, which means that if we detect an underrun we need to disable underrun
> > + * reporting on all pipes.
> > + *
> > + * The code also supports underrun detection on the PCH transcoder.
> > + */
> > +
> >  static bool ivb_can_enable_err_int(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -64,6 +84,14 @@ static bool cpt_can_enable_serr_int(struct drm_device *dev)
> >         return true;
> >  }
> >
> > +/**
> > + * i9xx_check_fifo_underruns - check for fifo underruns
> > + * @dev_priv: i915 device instance
> > + *
> > + * This function checks for fifo underruns on GMCH platforms. This needs to be
> > + * done manually on modeset to make sure that we catch all underruns since they
> > + * do not generate an interrupt by themselves on these platforms.
> > + */
> >  void i9xx_check_fifo_underruns(struct drm_i915_private *dev_priv)
> >  {
> >         struct intel_crtc *crtc;
> > @@ -199,20 +227,6 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> >         }
> >  }
> >
> > -/**
> > - * intel_set_cpu_fifo_underrun_reporting - enable/disable FIFO underrun messages
> > - * @dev: drm device
> > - * @pipe: pipe
> > - * @enable: true if we want to report FIFO underrun errors, false otherwise
> > - *
> > - * This function makes us disable or enable CPU fifo underruns for a specific
> > - * pipe. Notice that on some Gens (e.g. IVB, HSW), disabling FIFO underrun
> > - * reporting for one pipe may also disable all the other CPU error interruts for
> > - * the other pipes, due to the fact that there's just one interrupt mask/enable
> > - * bit for all the pipes.
> > - *
> > - * Returns the previous state of underrun reporting.
> > - */
> >  static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> >                                                     enum pipe pipe, bool enable)
> >  {
> > @@ -238,6 +252,22 @@ static bool __intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> >         return old;
> >  }
> >
> > +/**
> > + * intel_set_cpu_fifo_underrun_reporting - set cpu fifo underrrun reporting state
> > + * @dev_priv: i915 device instance
> > + * @pipe: (CPU) pipe to set state for
> > + * @enable: whether underruns should be reported or not
> > + *
> > + * This function sets the fifo underrun state for @pipe. It is used in the
> > + * modeset code to avoid false positives since on many platforms underruns are
> > + * expected when disabling or enabling the pipe.
> > + *
> > + * Notice that on some platforms disabling underrun reports for one pipe
> > + * disables for all due to shared interrupts. Actual reporting is still per-pipe
> > + * though.
> > + *
> > + * Returns the previous state of underrun reporting.
> > + */
> >  bool intel_set_cpu_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
> >                                            enum pipe pipe, bool enable)
> >  {
> > @@ -262,10 +292,10 @@ static bool __cpu_fifo_underrun_reporting_enabled(struct drm_i915_private *dev_p
> >  }
> >
> >  /**
> > - * intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun messages
> > - * @dev: drm device
> > + * intel_set_pch_fifo_underrun_reporting - set PCH fifo underrun reporting state
> > + * @dev_priv: i915 device instance
> >   * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
> > - * @enable: true if we want to report FIFO underrun errors, false otherwise
> > + * @enable: whether underruns should be reported or not
> >   *
> >   * This function makes us disable or enable PCH fifo underruns for a specific
> >   * PCH transcoder. Notice that on some PCHs (e.g. CPT/PPT), disabling FIFO
> > @@ -309,6 +339,15 @@ bool intel_set_pch_fifo_underrun_reporting(struct drm_i915_private *dev_priv,
> >         return old;
> >  }
> >
> > +/**
> > + * intel_pch_fifo_underrun_irq_handler - handle PCH fifo underrun interrup
> > + * @dev_priv: i915 device instance
> > + * @pipe: (CPU) pipe to set state for
> > + *
> > + * This handles a CPU fifo underrun interrupt, generating an underrun warning
> > + * into dmesg if underrun reporting is enabled and then disables the underrun
> > + * interrupt to avoid an irq storm.
> > + */
> >  void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> >                                          enum pipe pipe)
> >  {
> > @@ -322,6 +361,15 @@ void intel_cpu_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> >                           pipe_name(pipe));
> >  }
> >
> > +/**
> > + * intel_pch_fifo_underrun_irq_handler - handle PCH fifo underrun interrup
> > + * @dev_priv: i915 device instance
> > + * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
> > + *
> > + * This handles a PCH fifo underrun interrupt, generating an underrun warning
> > + * into dmesg if underrun reporting is enabled and then disables the underrun
> > + * interrupt to avoid an irq storm.
> > + */
> >  void intel_pch_fifo_underrun_irq_handler(struct drm_i915_private *dev_priv,
> >                                          enum transcoder pch_transcoder)
> >  {
> > --
> > 2.1.1
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch