On Mon, Dec 8, 2014 at 8:49 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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 ;-)
I agree, but I believe the right place is on subsequent patch that
changes to actually using frontbuffer tracking....
>
>> */
>>
>> +#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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
