From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay5-d.mail.gandi.net ([217.70.183.197]:58389 "EHLO relay5-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726865AbeIQSR7 (ORCPT ); Mon, 17 Sep 2018 14:17:59 -0400 Date: Mon, 17 Sep 2018 14:50:42 +0200 From: jacopo mondi To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, Ulrich Hecht , Kieran Bingham Subject: Re: [PATCH v2 06/16] drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get() Message-ID: <20180917125042.GK16851@w540> References: <20180914091046.483-1-laurent.pinchart+renesas@ideasonboard.com> <20180914091046.483-7-laurent.pinchart+renesas@ideasonboard.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AQNmCumFClRcGgHG" Content-Disposition: inline In-Reply-To: <20180914091046.483-7-laurent.pinchart+renesas@ideasonboard.com> Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: --AQNmCumFClRcGgHG Content-Type: text/plain; charset=utf-8 Content-Disposition: inline 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 > Tested-by: Jacopo Mondi Please add my Reviewed-by: Jacopo Mondi > --- > 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 > --AQNmCumFClRcGgHG Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJbn6MiAAoJEHI0Bo8WoVY8eGoQAMOCFWFNSUZVnFWLDLjE9ddu eHE23NtOn62cvvGehpPRqnJ+25iFKaKVhPfeoS1ZJHqqfqcnArVuKMYteE42sbK5 07vhE19kc//IVmIyvQ9nOWaUNJ1lZp5uh9+VDtBlEZTTcrKwIgEGQq1apuiLXguq 4Bz3R4A//+cqRF6ZTBkpp9Wt+8cVE4NHsU2Np2z7/g4IulfcyEk9Cu/r8XugCJBR MXIpg7iyQnDcNGwROWz4Uvvpv7iSE7B6ATvyDsjP/WqbYnwk4nuZi4zbsvPaHqu4 fkjdZMjy0qKKm+qOZoQHv0VxPyK08JlXgKxay+w7bPLtpRXskZEZYpOfn37PY7eD SqtcW/T7GRDtFaMj/eZcAmaGhCLmfBAvEmCgPvZZGaMTL/n94rRFlUObTbgutNq/ 9sdXJu3o/wIItQa/3hw4waURLMreGyG/Mhtf9CRQdakDF+eM4DXtEG4wWhBq0FUK GSUSAyH55muZoSBhqMqcyqPftjrkz4eKdoWS8K4LrCEVNBGQZuSlWo7dYZBSDf8x b1x507vQZyLsX7ivAlZKE4uSyU/ih1qb3tnXvPYRpAOaxoS9wwvzZCSI+2bn4afV jjQ58XlAHDgcFmv34KJfD5R/CRGCkUHxI72BE4R1TFBXFdroLeuVZFa8Ir6VTyZ1 YUmgh8ke3A/uKQHlUzz+ =Ipk+ -----END PGP SIGNATURE----- --AQNmCumFClRcGgHG-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: jacopo mondi Subject: Re: [PATCH v2 06/16] drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get() Date: Mon, 17 Sep 2018 14:50:42 +0200 Message-ID: <20180917125042.GK16851@w540> References: <20180914091046.483-1-laurent.pinchart+renesas@ideasonboard.com> <20180914091046.483-7-laurent.pinchart+renesas@ideasonboard.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0502633810==" Return-path: Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4769F6E290 for ; Mon, 17 Sep 2018 12:50:47 +0000 (UTC) In-Reply-To: <20180914091046.483-7-laurent.pinchart+renesas@ideasonboard.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: linux-renesas-soc@vger.kernel.org, Ulrich Hecht , Kieran Bingham , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0502633810== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AQNmCumFClRcGgHG" Content-Disposition: inline --AQNmCumFClRcGgHG Content-Type: text/plain; charset=utf-8 Content-Disposition: inline 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 > Tested-by: Jacopo Mondi Please add my Reviewed-by: Jacopo Mondi > --- > 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 > --AQNmCumFClRcGgHG Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJbn6MiAAoJEHI0Bo8WoVY8eGoQAMOCFWFNSUZVnFWLDLjE9ddu eHE23NtOn62cvvGehpPRqnJ+25iFKaKVhPfeoS1ZJHqqfqcnArVuKMYteE42sbK5 07vhE19kc//IVmIyvQ9nOWaUNJ1lZp5uh9+VDtBlEZTTcrKwIgEGQq1apuiLXguq 4Bz3R4A//+cqRF6ZTBkpp9Wt+8cVE4NHsU2Np2z7/g4IulfcyEk9Cu/r8XugCJBR MXIpg7iyQnDcNGwROWz4Uvvpv7iSE7B6ATvyDsjP/WqbYnwk4nuZi4zbsvPaHqu4 fkjdZMjy0qKKm+qOZoQHv0VxPyK08JlXgKxay+w7bPLtpRXskZEZYpOfn37PY7eD SqtcW/T7GRDtFaMj/eZcAmaGhCLmfBAvEmCgPvZZGaMTL/n94rRFlUObTbgutNq/ 9sdXJu3o/wIItQa/3hw4waURLMreGyG/Mhtf9CRQdakDF+eM4DXtEG4wWhBq0FUK GSUSAyH55muZoSBhqMqcyqPftjrkz4eKdoWS8K4LrCEVNBGQZuSlWo7dYZBSDf8x b1x507vQZyLsX7ivAlZKE4uSyU/ih1qb3tnXvPYRpAOaxoS9wwvzZCSI+2bn4afV jjQ58XlAHDgcFmv34KJfD5R/CRGCkUHxI72BE4R1TFBXFdroLeuVZFa8Ir6VTyZ1 YUmgh8ke3A/uKQHlUzz+ =Ipk+ -----END PGP SIGNATURE----- --AQNmCumFClRcGgHG-- --===============0502633810== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0502633810==--