Re: [PATCH 02/25] drm/i915/fbc: extract intel_fbc_can_activate()

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

Em Qui, 2016-01-21 às 12:35 +0100, Maarten Lankhorst escreveu:
> Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> > Extract all the code that checks if the FBC configuration is valid
> > to
> > its own function, making __intel_fbc_update() much simpler.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 92 ++++++++++++++++++++++----
> > --------------
> >  1 file changed, 50 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 6b43ec3..dff30e1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -514,17 +514,6 @@ static bool crtc_can_fbc(struct intel_crtc
> > *crtc)
> >  	return true;
> >  }
> >  
> > -static bool crtc_is_valid(struct intel_crtc *crtc)
> > -{
> > -	if (!intel_crtc_active(&crtc->base))
> > -		return false;
> > -	if (!to_intel_plane_state(crtc->base.primary->state)-
> > >visible)
> > -		return false;
> > -
> > -	return true;
> > -}
> > -
> >  static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
> >  {
> >  	enum pipe pipe;
> > @@ -750,48 +739,40 @@ static bool
> > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc)
> >  	return effective_w <= max_w && effective_h <= max_h;
> >  }
> >  
> > -/**
> > - * __intel_fbc_update - activate/deactivate FBC as needed,
> > unlocked
> > - * @crtc: the CRTC that triggered the update
> > - *
> > - * This function completely reevaluates the status of FBC, then
> > activates,
> > - * deactivates or maintains it on the same state.
> > - */
> > -static void __intel_fbc_update(struct intel_crtc *crtc)
> > +static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->base.dev-
> > >dev_private;
> > +	struct drm_plane *primary;
> >  	struct drm_framebuffer *fb;
> > +	struct intel_plane_state *plane_state;
> >  	struct drm_i915_gem_object *obj;
> >  	const struct drm_display_mode *adjusted_mode;
> It would be nice if this function had a plane_state and crtc_state
> passed into it for validation.
> I suppose it can wait though, maybe something for the future.

The way we handle this will change a few times during the series. In
the end this function will use the state cache, and
intel_fbc_update_state_cache() will be responsible for finding the
state structures.

> > -	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
> > -
> > -	if (!multiple_pipes_ok(dev_priv)) {
> > -		set_no_fbc_reason(dev_priv, "more than one pipe
> > active");
> > -		goto out_disable;
> > -	}
> > -
> > -	if (!dev_priv->fbc.enabled || dev_priv->fbc.crtc != crtc)
> > -		return;
> > -
> > -	if (!crtc_is_valid(crtc)) {
> > -		set_no_fbc_reason(dev_priv, "no output");
> > -		goto out_disable;
> > +	if (!intel_crtc_active(&crtc->base)) {
> > +		set_no_fbc_reason(dev_priv, "CRTC not active");
> > +		return false;
> >  	}
> This one has become redundant with plane_state->visible. You can't 
> have a visible plane with crtc off any more.

We fix this later in the series. If we just remove this right now, we
may have a NULL primary->fb causing a segfault, so we get rid of it
when the segfault won't be possible anymore.

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