On Wed, Nov 12, 2014 at 1:16 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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. I started the rebase yesterday putting it on begin of the
series than vbt work and at least 1 or 2 more coming.
Also I'll send along with vlv/chv rebased... Putting everything
organized together.
>
> 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.
I got vesa spec + our pm enabling guide and put the market names by
myself. Used to use market names on stack releases.
I believe that on docs that is visible to end users we could use more
market names besides the codenames.
>
> 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.
>> + */
Good. Thanks!
>> +
>> #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."?
definitely better. Thanks.
>> + */
>> 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.
noted.
>
>> + */
>> 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."
yeah. I'd have to change that for vlv/chv anyway where hw can never track.
thanks.
>
>> + */
>> 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."
noted
>> + */
>> 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/
noted
thank you very much for all suggestions.
>
>> + */
>> 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
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
