Re: [PATCH 02/11] drm/i915: Introduce FBC DocBook.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 08, 2014 at 02:09:11PM -0200, Paulo Zanoni wrote:
> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> No functional changes.
> 
> v2 (Paulo): Rebase.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Some suggestions to polish the documentation a bit below. I've merged
patch 1 right away to avoid further rebase pain.

Thanks, Daniel

> ---
>  Documentation/DocBook/drm.tmpl   |  5 ++++
>  drivers/gpu/drm/i915/intel_fbc.c | 57 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 85287cb..8b780ab 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3926,6 +3926,11 @@ int num_ioctls;</synopsis>
>  !Idrivers/gpu/drm/i915/intel_psr.c
>        </sect2>
>        <sect2>
> +	<title>Frame Buffer Compression (FBC)</title>
> +!Pdrivers/gpu/drm/i915/intel_fbc.c Frame Buffer Compression (FBC)
> +!Idrivers/gpu/drm/i915/intel_fbc.c
> +      </sect2>
> +      <sect2>
>          <title>DPIO</title>
>  !Pdrivers/gpu/drm/i915/i915_reg.h DPIO
>  	<table id="dpiox2">
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index f1eeb86..7686573 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -21,20 +21,31 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> -#include "intel_drv.h"
> -#include "i915_drv.h"
> -
> -/* FBC, or Frame Buffer Compression, is a technique employed to compress the
> - * framebuffer contents in-memory, aiming at reducing the required bandwidth
> +/**
> + * DOC: Frame Buffer Compression (FBC)
> + *
> + * FBC is a technique employed to compress the framebuffer contents
> + * in-memory, aiming at reducing the required bandwidth
>   * during in-memory transfers and, therefore, reduce the power packet.

The above paragraph imo doesn't make sense. And duplicates what's below,
so just delete it.
>   *
> + * FBC is primarily a memory power savings technology. That is the major
> + * benefit is to the memory power while displaying the processor graphics
> + * information to the display. FBC works by compressing the amount of memory
> + * used by the display. It means that it is total transparent to user space.

This reads a bit too much like marketing for my taste ;-) What about:

* FBC tries to save memory bandwidth (and so power consumption) by
* compressing the amount of memory used by the display. It is total
* transparent to user space and completely handled in the kernel.

> + *
>   * The benefits of FBC are mostly visible with solid backgrounds and
> - * variation-less patterns.
> + * variation-less patterns. It comes from keeping the memory footprint small
> + * and having fewer memory pages opened and accessed for refreshing the display.
>   *
> - * FBC-related functionality can be enabled by the means of the
> - * i915.i915_fbc_enable parameter
> + * i915 is responsible to reserve stolen memory for FBC and configure its
> + * offset on proper register. The hardware takes care of all

                       ^registers

> + * compress/decompress. However there are many known cases where we have to
> + * forcibly disable it to allow proper screen updates.

Mayb add "... using the frontbuffer tracking infrastructure." At least
when that's the case ;-)

>   */
>  
> +#include "intel_drv.h"
> +#include "i915_drv.h"
> +
>  static void i8xx_fbc_disable(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -318,6 +329,12 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
>  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
>  }
>  
> +/**
> + * intel_fbc_enabled - Is FBC enabled?
> + * @dev: the drm_device
> + *
> + * This function is used to verify the current state of FBC.

This needs a FIXME: This should be tracked in the plane config eventually
instead of queried at runtime for most callers.

> + */
>  bool intel_fbc_enabled(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -325,6 +342,18 @@ bool intel_fbc_enabled(struct drm_device *dev)
>  	return dev_priv->fbc.enabled;
>  }
>  
> +/**
> + * bdw_fbc_sw_flush - FBC Software Flush for Broadwell.
> + * @dev: the drm_device
> + * @value: Value to be set on MSG_FBC_REND_STATE. Possible values are
> + *         FBC_REND_NUKE and FBC_REND_CACHE_CLEAN.
> + *
> + * This function is needed on Broadwell to perform Nuke or Cache clean on
> + * software side over MMIO.
> + * On Broadwell, due a hardware bug, MSG_FBC_REND_STATE stay in a forbidden
> + * address that has a huge risk of causing GPU Hangs if set with LRI on some
> + * command streamers.
> + */

I guess with the frontbuffer tracking we'll just have invalidate/flush
entry points and this becomes a static function? In that case I wouldn't
bother documenting it - maybe do a comment referencing the wa name if
there is one.

>  void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -429,6 +458,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
>  	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
>  }
>  
> +/**
> + * intel_fbc_disable - disable FBC
> + * @dev: the drm_device
> + *
> + * This function disables FBC.
> + */
>  void intel_fbc_disable(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -643,6 +678,12 @@ out_disable:
>  	i915_gem_stolen_cleanup_compression(dev);
>  }
>  
> +/**
> + * intel_fbc_init - Initialize FBC
> + * @dev_priv: the i915 device
> + *
> + * This function might be called during PM init process.
> + */
>  void intel_fbc_init(struct drm_i915_private *dev_priv)
>  {
>  	if (!HAS_FBC(dev_priv)) {
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx