On Mon, Dec 08, 2014 at 02:09:11PM -0200, Paulo Zanoni wrote:
> From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> No functional changes.
>
> v2 (Paulo): Rebase.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
