Re: [PATCH 07/18] drm/i915: fix the __intel_fbc_update() comments

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qua, 2015-10-21 às 18:38 +0100, chris@chris-wilson.co.uk escreveu:
> 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@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.

The way I see, removing obsolete comments that list the reasons to
enable/disable FBC is orthogonal to the changes regarding
enable/disable/deactivate/activate.

Changing the patch order as you suggested will cause some rebase pain
just to achieve the same end result later, so unless there's some
strong objection I'll spare myself the pain.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx