On Fri, Nov 07, 2014 at 03:55:18PM -0800, Rodrigo Vivi wrote:
> Let's document PSR a bit. No functional changes.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
I've tried to merge the preceeding patches to avoid rebase pain, but
there's conflicts. I think it'd be best if we get those patches into shape
first. A few comments below to clarify the kerneldoc.
Thanks for doing this, docs are always highly appreciated!
-Daniel
> ---
> drivers/gpu/drm/i915/intel_psr.c | 58 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 793f491..5f350f0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -21,6 +21,26 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> +/**
> + * DOC: Panel Self Refresh (PSR/SRD)
> + *
> + * Since 4th Generation Intel® Core™ processors with Intel® HD Graphics
> + * (Haswell) Display controller supports Panel Self-Refresh on display panels
> + * witch have a remote frame buffer (RFB) implemented according to PSR spec in
> + * eDP1.3. PSR feature allows the display to go to lower standby states when
> + * system is idle but display is on as it eliminates diplay refresh request
> + * to DDR memory completely as long as the frame buffer for that display is
> + * unchanged.
> + *
> + * Panel Self Refresh must be supported by both Hardware (source) and
> + * Panel (sink).
> + *
> + * The PSR feature enables system-level power savings when the displayed image
> + * remains static for multiple display frames. The Sink device stores a static
> + * image locally in the RFB within the Sink device, and displays this image
> + * from the RFB, while the eDP Main-Link can be turned OFF.
This smells a lot like a marketing copy. Generally in code we don't use
marketing names but e.g. hsw.
Also it misses to explain the tricky implementation details, which is
what's actually important for the implementation. That psr has nice power
saving benefits is important, but I wouldn't put more than a sentence into
that, e.g.
"PSR saves power by caching the framebuffer in the panel, which allows us
to power down the link and memory controller. For DSI panels the same idea
is called "manual mode".
"The implementation uses the hardware-based PSR support which automatically
enters/exits self-refresh mode. The hardware takes care of sending the
required DP aux message and could even retrain the link (that part isn't
enabled yet though). The hardware also keeps track of any frontbuffer
changes to know when to exit self-refresh mode again. Unfortunately that
part doesn't work too well, hence why the i915 PSR support uses the
software frontbuffer tracking to make sure it doesn't miss a screen
update. For this integration intel_psr_invalidate() and intel_psr_flush()
get called by the frontbuffer tracking code. Note that because of locking
issues the self-refresh re-enable code is done from a work queue, which
must be correctly synchronized/cancelled when shutting down the pipe."
This way docs raise the tricky parts of the implementation and how/where
this integrates with the driver.
> + */
> +
> #include <drm/drmP.h>
>
> #include "intel_drv.h"
> @@ -222,6 +242,13 @@ static void intel_psr_do_enable(struct intel_dp *intel_dp)
> dev_priv->psr.active = true;
> }
>
> +/**
> + * intel_psr_enable - Enable PSR
> + * @intel_dp: Intel DP
> + *
> + * This function gets called every time eDP panel is turned on. Right after
> + * turning backlight on.
Does the backlight really matter? Should this be: "This can only be called
after the pipe is fully trained and enabled."?
> + */
> void intel_psr_enable(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -263,6 +290,13 @@ unlock:
> mutex_unlock(&dev_priv->psr.lock);
> }
>
> +/**
> + * intel_psr_disable - Disable PSR
> + * @intel_dp: Intel DP
> + *
> + * This function gets called every time eDP panel is turned off. Right before
> + * turning backlight off.
Same here.
> + */
> void intel_psr_disable(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -347,6 +381,16 @@ static void intel_psr_exit(struct drm_device *dev)
>
> }
>
> +/**
> + * intel_psr_invalidate - Invalidade PSR
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * Hardware (source) is usually the responsible for identifying when the screen
> + * changed and Remote Frame Buffer must be updated. However this HW tracking
> + * doesn't cover all cases. So we can take advantage of frontbuffer tracking
> + * to force a psr_exit when driver knows RFB must be updated.
Source hardware isn't responsible for tracking, this is just how it's
implemented on intel hw. You can do psr in software. What about:
"Since the hardware frontbuffer tracking has gaps we need to integrate
with the software frontbuffer tracking. This function gets called every
time frontbuffer rendering has completed and flushed out to memory. PSR
can be enabled again if no other frontbuffer relevant to PSR is dirty.
Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
> + */
> void intel_psr_invalidate(struct drm_device *dev,
> unsigned frontbuffer_bits)
> {
> @@ -371,6 +415,14 @@ void intel_psr_invalidate(struct drm_device *dev,
> mutex_unlock(&dev_priv->psr.lock);
> }
>
> +/**
> + * intel_psr_flush - Flush PSR
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * After PSR got invalidated forcing PSR exit and RFB update driver must also
> + * take care to flush it allowing PSR to come back to regular work.
"Since the hardware frontbuffer tracking has gaps we need to integrate
with the software frontbuffer tracking. This function gets called every
time frontbuffer rendering starts and a buffer gets dirtied. PSR must be
disabled if the frontbuffer mask contains a buffer relevant to PSR.
Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
> + */
> void intel_psr_flush(struct drm_device *dev,
> unsigned frontbuffer_bits)
> {
> @@ -404,6 +456,12 @@ void intel_psr_flush(struct drm_device *dev,
> mutex_unlock(&dev_priv->psr.lock);
> }
>
> +/**
> + * intel_psr_init - Init basic PSR work and mutex.
> + * @dev: DRM device
> + *
> + * This function is to be called only once to initialize basic PSR stuff.
s/to be// and maybe s/once/once at driver load/
> + */
> void intel_psr_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 1.9.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
