Re: [PATCH 0/8] eDP DRRS based on frontbuffer tracking

From: "Kannan, Vandana" <vandana.kannan@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 0/8] eDP DRRS based on frontbuffer tracking
Date: Mon, 15 Dec 2014 19:30:27 +0530	[thread overview]
Message-ID: <548EE97B.9050303@intel.com> (raw)
In-Reply-To: <20141215100050.GO27182@phenom.ffwll.local>


On 15-Dec-14 3:30 PM, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 02:22:49AM +0530, Vandana Kannan wrote:
>> This patch series inserts DRRS into frontbuffer tracking mechanism.
>>
>> 1. Previous submission for this feature was designed considering only eDP
>> DRRS. In this series, apart from following fb tracking, changes have been made
>> to make structures generic so that it can be of use to any other code
>> addition to support DRRS with other display types.
>> 2. DRRS support is checked based on VBT setting and panel's capability (if
>> more than one RR is supported).
>> 3. Based on DRRS support availability, related structures are initialized or
>> cleaned up through calls from enable/disable DDI respectively.
>> 4. Since flip() indicates busyness, changes have been made to invalidate
>> DRRS during flip. This changes RR back to preferred mode RR. New work to set
>> low RR is scheduled after a delay of x ms.
>> 5. This series includes patches to support RR switching on all platforms.
>> 6. A module param has been added, carrying forward the input from the
>> previous submission of the feature. This param indicates the delay in ms after
>> which a switch to low RR can be made. By default, this is set to 0
>> indicating that the feature is disabled.
>
> Looks good overall on a very cursory read. Some smaller comments in reply
> to individual patches. A few high-level things here:
> - I haven't fully checked the locking, so that still needs to be done in
>    the in-depth review. I'll also double-check after having applied
>    everything.
> - I think intel_dp_set_drrs_state doesn't need to be exported any with the
>    frontbuffer tracking implementation? Please add a patch to mark it
>    static and remove the header declaration.
I have made this change as part of patch 1/8..
> - Please add one patch with kerneldoc for the non-static DRRS functions
>    (_enable/disable and _flush/invalidate) and a short overview DOC:
>    section plus integration into drm.tmpl. See Paulo's in-flight fbc
>    patches and the work from Rodrigo with PSR for examples.
Ok sure..

- Vandana
> I think for detailed review someone who knows the frontbuffer tracking
> stuff well would be good. So please either pick Rodrigo or Paulo.
>
> Thanks, Daniel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx