Thank you for your patch.
> On September 14, 2018 at 11:10 AM Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> Tested-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> 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 6288b9ad9e24..c89751c26f9c 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)
> -{
> - 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
*balance
> + * 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
>
With typo fixed:
Reviewed-by: Ulrich Hecht <uli+renesas@xxxxxxxx>
CU
Uli
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
