[Intel-gfx] [PATCH 04/18] drm/i915: extract crtc_is_valid() on the FBC code

[chris at chris-wilson.co.uk]

On Wed, Oct 21, 2015 at 05:16:04PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-20 às 16:52 +0100, Chris Wilson escreveu:
> > On Tue, Oct 20, 2015 at 11:49:50AM -0200, Paulo Zanoni wrote:
> > > We're going to kill intel_fbc_find_crtc(), that's why a big part of
> > > the logic moved from intel_fbc_find_crtc() to crtc_is_valid().
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 26 ++++++++++++++++----------
> > >  1 file changed, 16 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index b9cfd16..1162787 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct
> > > drm_i915_private *dev_priv,
> > >  	DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
> > >  }
> > >  
> > > +static bool crtc_is_valid(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_i915_private *dev_priv = crtc->base.dev-
> > > >dev_private;
> > > +	enum pipe pipe = crtc->pipe;
> > > +
> > > +	if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >=
> > > 8) &&
> > > +	    pipe != PIPE_A)
> > 
> > Keeping
> > 
> > if (pipe_a_only(dev_priv) && pipe != PIPE_A)
> > 	return false;
> > 
> > would have been nicer.
> 
> I can do that on v2.
> 
> > 
> > > +		return false;
> > > +
> > > +	return intel_crtc_active(&crtc->base) &&
> > > +	       to_intel_plane_state(crtc->base.primary->state)-
> > > >visible &&
> > > +	       crtc->base.primary->fb != NULL;
> > 
> > And then you can split this line up for a little more clarity. If you
> > are taking the time to refactor into a separate function for
> > readability, you may as well apply a little polish as well.
> 
> I really don't get what you're suggesting here. Can you please clarify?

I think multiline conditionals do not add to readibilty. Since you have
already taken the effort to split the predicate out into its own
function,

  if (!intel_crtc_active(&crtc->base))
    return false;

  if (!to_intel_plane_state(crtc->base.primary->state)->visible)
    return false;

  /* both of these are used repeatedly in intel_display.c, probably worth
   * refactoring into intel_plane_active(crtc, crtc->base.primary); later
   * on
   */


  /* given the two above, this is redundant. more evidence that we should
   * not be writing this code ourselves but calling an intel_display
   * helper
   */
  if (crtc->base.primary->fb)
    return false;

  return true;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre