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

From: Daniel Vetter <daniel@ffwll.ch>
To: Vandana Kannan <vandana.kannan@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 0/8] eDP DRRS based on frontbuffer tracking
Date: Mon, 15 Dec 2014 11:00:50 +0100	[thread overview]
Message-ID: <20141215100050.GO27182@phenom.ffwll.local> (raw)
In-Reply-To: <1418244777-4964-1-git-send-email-vandana.kannan@intel.com>

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.
- 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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx