Re: [PATCH 4/4] drm/i915: Add PSR docbook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux