[Intel-gfx] [PATCH 07/18] drm/i915: fix the __intel_fbc_update() comments

[chris at chris-wilson.co.uk]

On Wed, Oct 21, 2015 at 05:32:16PM +0000, Zanoni, Paulo R wrote:
> Em Qua, 2015-10-21 às 13:37 +0100, Chris Wilson escreveu:
> > On Tue, Oct 20, 2015 at 11:49:53AM -0200, Paulo Zanoni wrote:
> > > Don't try to list in comments the cases where we should enable or
> > > disable FBC: it varies a lot with the hardware generations and the
> > > code should be the documentation. Also notice that there's already
> > > a
> > > huge gap between the comments and what's in the code.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 26 ++------------------------
> > >  1 file changed, 2 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index e856304..5fa4ce4 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -853,20 +853,8 @@ static bool
> > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> > >   * __intel_fbc_update - enable/disable FBC as needed, unlocked
> > >   * @dev_priv: i915 device instance
> > >   *
> > > - * Set up the framebuffer compression hardware at mode set
> > > time.  We
> > > - * enable it if possible:
> > > - *   - plane A only (on pre-965)
> > > - *   - no pixel mulitply/line duplication
> > > - *   - no alpha buffer discard
> > > - *   - no dual wide
> > > - *   - framebuffer <= max_hdisplay in width, max_vdisplay in
> > > height
> > > - *
> > > - * We can't assume that any compression will take place (worst
> > > case),
> > > - * so the compressed buffer has to be the same size as the
> > > uncompressed
> > > - * one.  It also must reside (along with the line length buffer)
> > > in
> > > - * stolen memory.
> > > - *
> > > - * We need to enable/disable FBC on a global basis.
> > > + * This function completely reevaluates the status of FBC, then
> > > enables,
> > > + * disables or maintains it on the same state.
> > 
> > By the end, this comment is not strictly true ,right? Since you move
> > some of the status checking into modeset enable paths. Could you
> > refine
> > the function comment?
> 
> I only move the status checking into modeset enable paths on patch 12.
> The comment above is modified on patch 12 to reflect
> activate/deactivate instead of enable/disable.

Honestly, I would just squash this patch, or apply it after the moving
the code to correctly reflect the split in work between update/enable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre