From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [85.220.165.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2D2952C81 for ; Thu, 28 Oct 2021 11:15:52 +0000 (UTC) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mg3Nl-0006KA-0k; Thu, 28 Oct 2021 13:15:41 +0200 Received: from [2a0a:edc0:0:900:1d::77] (helo=ptz.office.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtp (Exim 4.92) (envelope-from ) id 1mg3Ng-0002Rz-44; Thu, 28 Oct 2021 13:15:36 +0200 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1mg3Ng-00041g-2p; Thu, 28 Oct 2021 13:15:36 +0200 Date: Thu, 28 Oct 2021 13:15:35 +0200 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sean Young Cc: =?utf-8?B?TWHDrXJh?= Canal , lkp@intel.com, mchehab@kernel.org, thierry.reding@gmail.com, lee.jones@linaro.org, llvm@lists.linux.dev, kbuild-all@lists.01.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org Subject: Re: [PATCH v4] media: rc: pwm-ir-tx: Switch to atomic PWM API Message-ID: <20211028111535.x7xgz7domx2lpyfh@pengutronix.de> References: <20211028064513.guziv6uaivzlk6ki@pengutronix.de> <20211028091442.GA16514@gofer.mess.org> Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="gairqvahdkp57zyj" Content-Disposition: inline In-Reply-To: <20211028091442.GA16514@gofer.mess.org> X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: llvm@lists.linux.dev --gairqvahdkp57zyj Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hello Sean, On Thu, Oct 28, 2021 at 10:14:42AM +0100, Sean Young wrote: > On Thu, Oct 28, 2021 at 08:45:13AM +0200, Uwe Kleine-K=F6nig wrote: > > The conversion is right (I think), >=20 > We still have the problem that the pwm drivers calculate the period > incorrectly by rounding down (except pwm-bcm2835). So the period is not > as good as it could be in most cases, but this driver can't do anything > about that. Yeah, some time ago I started coding a round_state function (wip at https://git.pengutronix.de/cgit/ukl/linux/commit/?h=3Dpwm-wip&id=3Dae348eb6= a55d6526f30ef4a49819197d9616391e) but this was pushed down on my todo-list by more important stuff. If you want to experiment with that ... > > note this could be optimized a bit > > further: state.period only depends on carrier which rarely changes, so > > the calculation could be done in pwm_ir_set_carrier(). Ditto for duty > > which only depends on state.period and pwm_ir->duty_cycle. (This is for > > a separate commit though.) >=20 > I'm not sure what caching this is much of a win. The calculation is a few > instructions, so you're not winning in the way of speed. On the flip side > you use more memory since pwm_state has to be kmalloc() rather than exist= ing I tested a bit with this patch on top of Ma=EDra's: diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c index 105a9c24f1e3..7585c21775bc 100644 --- a/drivers/media/rc/pwm-ir-tx.c +++ b/drivers/media/rc/pwm-ir-tx.c @@ -17,7 +17,7 @@ =20 struct pwm_ir { struct pwm_device *pwm; - unsigned int carrier; + struct pwm_state state; unsigned int duty_cycle; }; =20 @@ -32,6 +32,7 @@ static int pwm_ir_set_duty_cycle(struct rc_dev *dev, u32= duty_cycle) struct pwm_ir *pwm_ir =3D dev->priv; =20 pwm_ir->duty_cycle =3D duty_cycle; + pwm_set_relative_duty_cycle(&pwm_ir->state, pwm_ir->duty_cycle, 100); =20 return 0; } @@ -43,7 +44,8 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, u32 ca= rrier) if (!carrier) return -EINVAL; =20 - pwm_ir->carrier =3D carrier; + pwm_ir->state.period =3D DIV_ROUND_CLOSEST(NSEC_PER_SEC, carrier); + pwm_set_relative_duty_cycle(&pwm_ir->state, pwm_ir->duty_cycle, 100); =20 return 0; } @@ -53,21 +55,15 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int = *txbuf, { struct pwm_ir *pwm_ir =3D dev->priv; struct pwm_device *pwm =3D pwm_ir->pwm; - struct pwm_state state; int i; ktime_t edge; long delta; =20 - pwm_init_state(pwm, &state); - - state.period =3D DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier); - pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100); - edge =3D ktime_get(); =20 for (i =3D 0; i < count; i++) { - state.enabled =3D !(i % 2); - pwm_apply_state(pwm, &state); + pwm_ir->state.enabled =3D !(i % 2); + pwm_apply_state(pwm, &pwm_ir->state); =20 edge =3D ktime_add_us(edge, txbuf[i]); delta =3D ktime_us_delta(edge, ktime_get()); @@ -75,8 +71,8 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *t= xbuf, usleep_range(delta, delta + 10); } =20 - state.enabled =3D false; - pwm_apply_state(pwm, &state); + pwm_ir->state.enabled =3D false; + pwm_apply_state(pwm, &pwm_ir->state); =20 return count; } @@ -95,8 +91,9 @@ static int pwm_ir_probe(struct platform_device *pdev) if (IS_ERR(pwm_ir->pwm)) return PTR_ERR(pwm_ir->pwm); =20 - pwm_ir->carrier =3D 38000; - pwm_ir->duty_cycle =3D 50; + pwm_ir->state.duty_cycle =3D 50; + pwm_init_state(pwm_ir->pwm, &pwm_ir->state); + pwm_ir_set_carrier(rcdev, 38000); =20 rcdev =3D devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX); if (!rcdev) bloat-o-meter reports (for an arm allmodconfig build) add/remove: 0/0 grow/shrink: 3/1 up/down: 644/-396 (248) Function old new delta pwm_ir_probe 372 676 +304 pwm_ir_set_carrier 108 292 +184 pwm_ir_set_duty_cycle 68 224 +156 pwm_ir_tx 908 512 -396 Total: Before=3D2302, After=3D2550, chg +10.77% struct pwm_ir increases from 12 bytes to 40 bytes. The stack space required by pwm_ir_tx decreases from 60 to 36 I don't know exactly how kmalloc works internally. Maybe allocating a structure of size 40 bytes doesn't need more memory than a structure of size 12? I didn't check how runtimes change, but the size decrease of pwm_ir_tx() is nice and might save a bit of runtime. Probably it depends on your focus if this change is good for you or not. > just on the stack, and both ioctl handlers and the probe function need to > recalculate the period/duty cycle, so there is a slight increase in code = size. >=20 > This change does not improve anything measurably and only increases code > complexity. You did measure? Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --gairqvahdkp57zyj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmF6hlQACgkQwfwUeK3K 7AlKtwf9H5aw8l3EXGUcjZhfwd8EYuoy+PO1ri2AL13Q9N1wUHR7f+qJNX0ltBh1 VFSdscpayWWZD5fcjWYZljPFemWuSN/2j4lw/gTCghoO0Wy6EbyNBX0cSFEcQdm4 Q1RnsbmrJnHdn9U4CiaOxrHI8MwlsbIy5vebdzLBqUTgGAV/yPXhxJ1EOMj2HE3a xzfWBQJ8V6Mj57iZjWwluoGD4SkfxDQxTz815ZPeQ04E6W3eRDrBY102Ao8lJas7 WR1zN5r2z+vrN0zHWdYmgojaqPw0HfRWWIWVVk9GdxjPkJ2HF8F0h5Se9CzLp9ib ekSOh4JKwrxxSo9GwN6tpNJc/yqHWg== =MAUv -----END PGP SIGNATURE----- --gairqvahdkp57zyj-- From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============1307592205104776397==" MIME-Version: 1.0 From: Uwe Kleine-König To: kbuild-all@lists.01.org Subject: Re: [PATCH v4] media: rc: pwm-ir-tx: Switch to atomic PWM API Date: Thu, 28 Oct 2021 13:15:35 +0200 Message-ID: <20211028111535.x7xgz7domx2lpyfh@pengutronix.de> In-Reply-To: <20211028091442.GA16514@gofer.mess.org> List-Id: --===============1307592205104776397== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hello Sean, On Thu, Oct 28, 2021 at 10:14:42AM +0100, Sean Young wrote: > On Thu, Oct 28, 2021 at 08:45:13AM +0200, Uwe Kleine-K=C3=B6nig wrote: > > The conversion is right (I think), > = > We still have the problem that the pwm drivers calculate the period > incorrectly by rounding down (except pwm-bcm2835). So the period is not > as good as it could be in most cases, but this driver can't do anything > about that. Yeah, some time ago I started coding a round_state function (wip at https://git.pengutronix.de/cgit/ukl/linux/commit/?h=3Dpwm-wip&id=3Dae348eb6= a55d6526f30ef4a49819197d9616391e) but this was pushed down on my todo-list by more important stuff. If you want to experiment with that ... > > note this could be optimized a bit > > further: state.period only depends on carrier which rarely changes, so > > the calculation could be done in pwm_ir_set_carrier(). Ditto for duty > > which only depends on state.period and pwm_ir->duty_cycle. (This is for > > a separate commit though.) > = > I'm not sure what caching this is much of a win. The calculation is a few > instructions, so you're not winning in the way of speed. On the flip side > you use more memory since pwm_state has to be kmalloc() rather than exist= ing I tested a bit with this patch on top of Ma=C3=ADra's: diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c index 105a9c24f1e3..7585c21775bc 100644 --- a/drivers/media/rc/pwm-ir-tx.c +++ b/drivers/media/rc/pwm-ir-tx.c @@ -17,7 +17,7 @@ = struct pwm_ir { struct pwm_device *pwm; - unsigned int carrier; + struct pwm_state state; unsigned int duty_cycle; }; = @@ -32,6 +32,7 @@ static int pwm_ir_set_duty_cycle(struct rc_dev *dev, u32= duty_cycle) struct pwm_ir *pwm_ir =3D dev->priv; = pwm_ir->duty_cycle =3D duty_cycle; + pwm_set_relative_duty_cycle(&pwm_ir->state, pwm_ir->duty_cycle, 100); = return 0; } @@ -43,7 +44,8 @@ static int pwm_ir_set_carrier(struct rc_dev *dev, u32 ca= rrier) if (!carrier) return -EINVAL; = - pwm_ir->carrier =3D carrier; + pwm_ir->state.period =3D DIV_ROUND_CLOSEST(NSEC_PER_SEC, carrier); + pwm_set_relative_duty_cycle(&pwm_ir->state, pwm_ir->duty_cycle, 100); = return 0; } @@ -53,21 +55,15 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int = *txbuf, { struct pwm_ir *pwm_ir =3D dev->priv; struct pwm_device *pwm =3D pwm_ir->pwm; - struct pwm_state state; int i; ktime_t edge; long delta; = - pwm_init_state(pwm, &state); - - state.period =3D DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier); - pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100); - edge =3D ktime_get(); = for (i =3D 0; i < count; i++) { - state.enabled =3D !(i % 2); - pwm_apply_state(pwm, &state); + pwm_ir->state.enabled =3D !(i % 2); + pwm_apply_state(pwm, &pwm_ir->state); = edge =3D ktime_add_us(edge, txbuf[i]); delta =3D ktime_us_delta(edge, ktime_get()); @@ -75,8 +71,8 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *t= xbuf, usleep_range(delta, delta + 10); } = - state.enabled =3D false; - pwm_apply_state(pwm, &state); + pwm_ir->state.enabled =3D false; + pwm_apply_state(pwm, &pwm_ir->state); = return count; } @@ -95,8 +91,9 @@ static int pwm_ir_probe(struct platform_device *pdev) if (IS_ERR(pwm_ir->pwm)) return PTR_ERR(pwm_ir->pwm); = - pwm_ir->carrier =3D 38000; - pwm_ir->duty_cycle =3D 50; + pwm_ir->state.duty_cycle =3D 50; + pwm_init_state(pwm_ir->pwm, &pwm_ir->state); + pwm_ir_set_carrier(rcdev, 38000); = rcdev =3D devm_rc_allocate_device(&pdev->dev, RC_DRIVER_IR_RAW_TX); if (!rcdev) bloat-o-meter reports (for an arm allmodconfig build) add/remove: 0/0 grow/shrink: 3/1 up/down: 644/-396 (248) Function old new delta pwm_ir_probe 372 676 +304 pwm_ir_set_carrier 108 292 +184 pwm_ir_set_duty_cycle 68 224 +156 pwm_ir_tx 908 512 -396 Total: Before=3D2302, After=3D2550, chg +10.77% struct pwm_ir increases from 12 bytes to 40 bytes. The stack space required by pwm_ir_tx decreases from 60 to 36 I don't know exactly how kmalloc works internally. Maybe allocating a structure of size 40 bytes doesn't need more memory than a structure of size 12? I didn't check how runtimes change, but the size decrease of pwm_ir_tx() is nice and might save a bit of runtime. Probably it depends on your focus if this change is good for you or not. > just on the stack, and both ioctl handlers and the probe function need to > recalculate the period/duty cycle, so there is a slight increase in code = size. > = > This change does not improve anything measurably and only increases code > complexity. You did measure? Best regards Uwe -- = Pengutronix e.K. | Uwe Kleine-K=C3=B6nig = | Industrial Linux Solutions | https://www.pengutronix.de/ | --===============1307592205104776397== Content-Type: application/pgp-signature MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRRXpCQUFCQ2dBZEZpRUVmbklxRnBBWXJQ OCtkS1FMd2Z3VWVLM0s3QWtGQW1GNmhsUUFDZ2tRd2Z3VWVLM0sKN0FsS3R3ZjlINWF3OGwzRVhH VWNqWmhmd2Q4RVl1b3krUE8xcmkyQUwxM1E5TjF3VUhSN2YrcUpOWDBsdEJoMQpWRlNkc2NwYXlX V1pENWZjaldZWmxqUEZlbVd1U04vMmo0bHcvZ1RDZ2hvTzBXeTZFYnlOQlgwY1NGRWNRZG00ClEx Um5zYm1ySm5IZG45VTRDaWFPeHJISThNd2xzYkl5NXZlYmR6TEJxVVRnR0FWL3lQWGh4SjFFT01q MkhFM2EKeHpmV0JRSjhWNk1qNTdpWmpXd2x1b0dENFNrZnhEUXhUejgxNVpQZVEwNEU2VzNlUkRy QlkxMDJBbzhsSmFzNwpXUjF6TjVyMnordnJOMHpIV2RZbWdvamFxUHcwSGZSV1dJV1ZWazlHZHhq UGtKMkhGOEYwaDVTZTlDekxwOWliCmVrU09oNEpLd3J4eFNvOUd3TjZ0cE5KYy95cUhXZz09Cj1N QVV2Ci0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo= --===============1307592205104776397==--