Re: [PATCH 06/16] drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Friday, 7 September 2018 21:19:38 EEST jacopo mondi wrote:
> On Tue, Sep 04, 2018 at 03:10:17PM +0300, Laurent Pinchart wrote:
> > The rcar_du_crtc_get() function is always immediately followed by a call
> > to rcar_du_crtc_setup(). Call the later from the former to simplify the
> > code, and add a comment to explain how the get and put calls are
> > balanced.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 107 +++++++++++++++-------------
> >  1 file changed, 56 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 4c9572d7ea89..1d81eb244441
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -66,39 +66,6 @@ static void rcar_du_crtc_clr_set(struct rcar_du_crtc
> > *rcrtc, u32 reg,
> >  	rcar_du_write(rcdu, rcrtc->mmio_offset + reg, (value & ~clr) | set);
> >  }
> > 
> > -static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
> > -{
> > -	int ret;
> > -
> > -	ret = clk_prepare_enable(rcrtc->clock);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	ret = clk_prepare_enable(rcrtc->extclock);
> > -	if (ret < 0)
> > -		goto error_clock;
> > -
> > -	ret = rcar_du_group_get(rcrtc->group);
> > -	if (ret < 0)
> > -		goto error_group;
> > -
> > -	return 0;
> > -
> > -error_group:
> > -	clk_disable_unprepare(rcrtc->extclock);
> > -error_clock:
> > -	clk_disable_unprepare(rcrtc->clock);
> > -	return ret;
> > -}
> > -
> > -static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
> 
> For what I see, rcar_du_crtc_stop() is also called once just before
> _put(). Have you considered doing with them what you've done with
> _get() and _setup() ?

I have, but given that we have an explicit get(); start(); sequence in 
rcar_du_crtc_atomic_enable(), I'd rather keep the stop(); put(); sequence in 
rcar_du_crtc_atomic_disable().

> > -{
> > -	rcar_du_group_put(rcrtc->group);
> > -
> > -	clk_disable_unprepare(rcrtc->extclock);
> > -	clk_disable_unprepare(rcrtc->clock);
> > -}
> > -
> > 
> >  /*
> >  ------------------------------------------------------------------------
> >  ----->  
> >   * Hardware Setup
> >   */
> > 
> > @@ -546,6 +513,51 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc
> > *rcrtc)> 
> >  	drm_crtc_vblank_on(&rcrtc->crtc);
> >  
> >  }
> > 
> > +static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * Guard against double-get, as the function is called from both the
> > +	 * .atomic_enable() and .atomic_begin() handlers.
> > +	 */
> > +	if (rcrtc->initialized)
> > +		return 0;
> > +
> > +	ret = clk_prepare_enable(rcrtc->clock);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(rcrtc->extclock);
> > +	if (ret < 0)
> > +		goto error_clock;
> > +
> > +	ret = rcar_du_group_get(rcrtc->group);
> > +	if (ret < 0)
> > +		goto error_group;
> > +
> > +	rcar_du_crtc_setup(rcrtc);
> > +	rcrtc->initialized = true;
> > +
> > +	return 0;
> > +
> > +error_group:
> > +	clk_disable_unprepare(rcrtc->extclock);
> > +error_clock:
> > +	clk_disable_unprepare(rcrtc->clock);
> > +	return ret;
> > +}
> > +
> > +static void rcar_du_crtc_put(struct rcar_du_crtc *rcrtc)
> > +{
> > +	rcar_du_group_put(rcrtc->group);
> > +
> > +	clk_disable_unprepare(rcrtc->extclock);
> > +	clk_disable_unprepare(rcrtc->clock);
> > +
> > +	rcrtc->initialized = false;
> > +}
> > +
> > 
> >  static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> >  {
> >  
> >  	bool interlaced;
> > 
> > @@ -639,16 +651,7 @@ static void rcar_du_crtc_atomic_enable(struct
> > drm_crtc *crtc,> 
> >  {
> >  
> >  	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > 
> > -	/*
> > -	 * If the CRTC has already been setup by the .atomic_begin() handler we
> > -	 * can skip the setup stage.
> > -	 */
> > -	if (!rcrtc->initialized) {
> > -		rcar_du_crtc_get(rcrtc);
> > -		rcar_du_crtc_setup(rcrtc);
> > -		rcrtc->initialized = true;
> > -	}
> > -
> > +	rcar_du_crtc_get(rcrtc);
> > 
> >  	rcar_du_crtc_start(rcrtc);
> >  
> >  }
> > 
> > @@ -667,7 +670,6 @@ static void rcar_du_crtc_atomic_disable(struct
> > drm_crtc *crtc,> 
> >  	}
> >  	spin_unlock_irq(&crtc->dev->event_lock);
> > 
> > -	rcrtc->initialized = false;
> > 
> >  	rcrtc->outputs = 0;
> >  
> >  }
> > 
> > @@ -680,14 +682,17 @@ static void rcar_du_crtc_atomic_begin(struct
> > drm_crtc *crtc,> 
> >  	/*
> >  	
> >  	 * If a mode set is in progress we can be called with the CRTC 
disabled.
> > 
> > -	 * We then need to first setup the CRTC in order to configure planes.
> > -	 * The .atomic_enable() handler will notice and skip the CRTC setup.
> > +	 * We thus need to first get and setup the CRTC in order to configure
> > +	 * planes. We must *not* put the CRTC in .atomic_flush(), as it must be
> > +	 * kept awake until the .atomic_enable() call that will follow. The get
> > +	 * operation in .atomic_enable() will in that case be a no-op, and the
> > +	 * CRTC will be put later in .atomic_disable().
> > +	 *
> > +	 * If a mode set is not in progress the CRTC is enabled, and the
> > +	 * following get call will be a no-op. There is thus no need to belance
> > +	 * it in .atomic_flush() either.
> > 
> >  	 */
> > 
> > -	if (!rcrtc->initialized) {
> > -		rcar_du_crtc_get(rcrtc);
> > -		rcar_du_crtc_setup(rcrtc);
> > -		rcrtc->initialized = true;
> > -	}
> > +	rcar_du_crtc_get(rcrtc);
> > 
> >  	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> >  	
> >  		rcar_du_vsp_atomic_begin(rcrtc);


-- 
Regards,

Laurent Pinchart