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

Commit Message

Paulo Zanoni Dec. 8, 2014, 4:09 p.m. UTC

From: Rodrigo Vivi <[e-mail]>

No functional changes.

v2 (Paulo): Rebase.

Signed-off-by: Rodrigo Vivi <[e-mail]>
Signed-off-by: Paulo Zanoni <[e-mail]>
---
 Documentation/DocBook/drm.tmpl   |  5 ++++
 drivers/gpu/drm/i915/intel_fbc.c | 57 ++++++++++++++++++++++++++++++++++------
 2 files changed, 54 insertions(+), 8 deletions(-)

Comments

Daniel Vetter Dec. 8, 2014, 4:49 p.m. UTC | #1

On Mon, Dec 08, 2014 at 02:09:11PM -0200, Paulo Zanoni wrote:
> From: Rodrigo Vivi <[e-mail]>
> 
> No functional changes.
> 
> v2 (Paulo): Rebase.
> 
> Signed-off-by: Rodrigo Vivi <[e-mail]>
> Signed-off-by: Paulo Zanoni <[e-mail]>

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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi Dec. 8, 2014, 9:48 p.m. UTC | #2

On Mon, Dec 8, 2014 at 8:49 AM, Daniel Vetter <[e-mail]> wrote:
> On Mon, Dec 08, 2014 at 02:09:11PM -0200, Paulo Zanoni wrote:
>> From: Rodrigo Vivi <[e-mail]>
>>
>> No functional changes.
>>
>> v2 (Paulo): Rebase.
>>
>> Signed-off-by: Rodrigo Vivi <[e-mail]>
>> Signed-off-by: Paulo Zanoni <[e-mail]>
>
> 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
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter Dec. 9, 2014, 9:52 a.m. UTC | #3

On Mon, Dec 08, 2014 at 01:48:01PM -0800, Rodrigo Vivi wrote:
> On Mon, Dec 8, 2014 at 8:49 AM, Daniel Vetter <[e-mail]> wrote:
> > On Mon, Dec 08, 2014 at 02:09:11PM -0200, Paulo Zanoni wrote:
> >> + * 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....

Make sense and thanks for the updated doc patch, merged to dinq.

Paulo, can you please make a note to add this to the patch which switches
to frontbuffer tracking?

Thanks, Daniel

Patch

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.
  *
+ * 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.
+ *
  * 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
+ * compress/decompress. However there are many known cases where we have to
+ * forcibly disable it to allow proper screen updates.
  */
 
+#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.
+ */
 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.
+ */
 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)) {