| Message ID | 20180914091046.483-7-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | R-Car D3/E3 display support (with LVDS PLL) | expand |
Hi Laurent, On Fri, Sep 14, 2018 at 12:10:36PM +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> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Please add my Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > 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 > + * 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 >
Thank you for your patch. > On September 14, 2018 at 11:10 AM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> 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> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > 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@fpond.eu> CU Uli
Hi Ulrich, Thank you for the review. Patches have however landed upstream already, so I can't add any Reviewed-by tag anymore. I will submit a follow-up patch, unless you would prefer doing it yourself. On Wednesday, 26 September 2018 18:55:14 EEST Ulrich Hecht wrote: > Thank you for your patch. > > > On September 14, 2018 at 11:10 AM Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> 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> > > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > > > 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); > > With typo fixed: > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
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 + * 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);