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 F38022C83 for ; Fri, 29 Oct 2021 11:06:32 +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 1mgPiK-0001mx-Nh; Fri, 29 Oct 2021 13:06:24 +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 1mgPiH-0005QG-KK; Fri, 29 Oct 2021 13:06:21 +0200 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1mgPiH-0002TX-J4; Fri, 29 Oct 2021 13:06:21 +0200 Date: Fri, 29 Oct 2021 13:06:02 +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: <20211029110602.uugnbm5vtfpghiwh@pengutronix.de> References: <20211028064513.guziv6uaivzlk6ki@pengutronix.de> <20211028091442.GA16514@gofer.mess.org> <20211028111535.x7xgz7domx2lpyfh@pengutronix.de> <20211028122610.GA18767@gofer.mess.org> <20211028180516.t2tpfbzztm7s6cqm@pengutronix.de> <20211029071608.GA28997@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="azgvopiwgpdhztkw" Content-Disposition: inline In-Reply-To: <20211029071608.GA28997@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 --azgvopiwgpdhztkw Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 29, 2021 at 08:16:08AM +0100, Sean Young wrote: > On Thu, Oct 28, 2021 at 08:05:16PM +0200, Uwe Kleine-K=F6nig wrote: > > On Thu, Oct 28, 2021 at 01:26:10PM +0100, Sean Young wrote: > > > > bloat-o-meter reports (for an arm allmodconfig build) > > > >=20 > > > > 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% > > >=20 > > > So 248 bytes more after your changes. > >=20 > > ack. This is because the compiler inlines the division which accounts > > for > 100 bytes. >=20 > I'm surprised it's that large. This is on 32 bit? Yes, it's a 64 bit division on 32 bit ARM. > > > > struct pwm_ir increases from 12 bytes to 40 bytes. > > > >=20 > > > > The stack space required by pwm_ir_tx decreases from 60 to 36 > > > >=20 > > > > I don't know exactly how kmalloc works internally. Maybe allocating= a > > > > structure of size 40 bytes doesn't need more memory than a structur= e of > > > > size 12? > > > >=20 > > > > I didn't check how runtimes change, but the size decrease of pwm_ir= _tx() > > > > is nice and might save a bit of runtime. > > >=20 > > > I'm not following, how is this decreasing runtime?=20 > >=20 > > With my changes pwm_ir_tx got smaller and { pwm_ir_probe, > > pwm_ir_set_carrier, pwm_ir_set_duty_cycle } got bigger. Now if for a > > typical runtime pattern pwm_ir_probe and pwm_ir_set_carrier run once and > > pwm_ir_set_duty_cycle 100 times and pwm_ir_tx 1000 times (no idea if > > that is realistic) it might be a net win in sum. >=20 > The two most common programs for sending IR are >=20 > ir-ctl: https://git.linuxtv.org/v4l-utils.git/tree/utils/ir-ctl/ir-ctl.c#= n1041 > lircd: https://sourceforge.net/p/lirc/git/ci/master/tree/lib/transmit.c >=20 > For each transmission, the carrier is set. If the duty cyle is specified, > then that is set too. Then the transmit itself is done. Both of them > set the carrier and duty cycle (if required) for every transmission: sett= ing > the carrier and duty cycle is a cheap operation, and it is device property > which can be overriden by another process.=20 >=20 > This means with your changes, if the carrier and duty cycle are both set > for each transmission, then we're doing more work. If only the carrier > is set for each transmission, then there is no net gain/loss (I think), > but the code size has increased. OK, then I discard my patch. While reading that I wondered if it makes sense to have a callback that sets both carrier and duty cycle and then remove the other two. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --azgvopiwgpdhztkw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmF71ZIACgkQwfwUeK3K 7Am79wf/SqQB9cLC/swxCjRZ+kXJ+3z7js+zIjZQHScbxNaycBfosNwROYJ/U5IJ ArOIy5R7/y6PzPwE+iPEIYXA/o7tz/wamOogZWcT8ZxJVC/UlSkcx5Sh7z0xAkbC ISbuy4VtDhOp0+BL8g4VasCYf1yYs8SFdzcKw76FgYM4/Qmr8VRuukYL21SbHfWV O+W3G/MZE5NLV79x33fsPNkfvYiJav2xF1IYKde4dyJGG3HKd5uS3OGo8xU6rhpR WUfLjldheUvP4ZVO0HcGPJTHia1K8bFB1qfwkYDuetBhpICHk+mbpWP92OfBvtJo 2/en1tqciILjZz/+eRRRa9kTnehVWg== =3f91 -----END PGP SIGNATURE----- --azgvopiwgpdhztkw-- From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============0856864945193865002==" 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: Fri, 29 Oct 2021 13:06:02 +0200 Message-ID: <20211029110602.uugnbm5vtfpghiwh@pengutronix.de> In-Reply-To: <20211029071608.GA28997@gofer.mess.org> List-Id: --===============0856864945193865002== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Fri, Oct 29, 2021 at 08:16:08AM +0100, Sean Young wrote: > On Thu, Oct 28, 2021 at 08:05:16PM +0200, Uwe Kleine-K=C3=B6nig wrote: > > On Thu, Oct 28, 2021 at 01:26:10PM +0100, Sean Young wrote: > > > > 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% > > > = > > > So 248 bytes more after your changes. > > = > > ack. This is because the compiler inlines the division which accounts > > for > 100 bytes. > = > I'm surprised it's that large. This is on 32 bit? Yes, it's a 64 bit division on 32 bit ARM. > > > > 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 structur= e 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. > > > = > > > I'm not following, how is this decreasing runtime? = > > = > > With my changes pwm_ir_tx got smaller and { pwm_ir_probe, > > pwm_ir_set_carrier, pwm_ir_set_duty_cycle } got bigger. Now if for a > > typical runtime pattern pwm_ir_probe and pwm_ir_set_carrier run once and > > pwm_ir_set_duty_cycle 100 times and pwm_ir_tx 1000 times (no idea if > > that is realistic) it might be a net win in sum. > = > The two most common programs for sending IR are > = > ir-ctl: https://git.linuxtv.org/v4l-utils.git/tree/utils/ir-ctl/ir-ctl.c#= n1041 > lircd: https://sourceforge.net/p/lirc/git/ci/master/tree/lib/transmit.c > = > For each transmission, the carrier is set. If the duty cyle is specified, > then that is set too. Then the transmit itself is done. Both of them > set the carrier and duty cycle (if required) for every transmission: sett= ing > the carrier and duty cycle is a cheap operation, and it is device property > which can be overriden by another process. = > = > This means with your changes, if the carrier and duty cycle are both set > for each transmission, then we're doing more work. If only the carrier > is set for each transmission, then there is no net gain/loss (I think), > but the code size has increased. OK, then I discard my patch. While reading that I wondered if it makes sense to have a callback that sets both carrier and duty cycle and then remove the other two. Best regards Uwe -- = Pengutronix e.K. | Uwe Kleine-K=C3=B6nig = | Industrial Linux Solutions | https://www.pengutronix.de/ | --===============0856864945193865002== Content-Type: application/pgp-signature MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRRXpCQUFCQ2dBZEZpRUVmbklxRnBBWXJQ OCtkS1FMd2Z3VWVLM0s3QWtGQW1GNzFaSUFDZ2tRd2Z3VWVLM0sKN0FtNzl3Zi9TcVFCOWNMQy9z d3hDalJaK2tYSiszejdqcyt6SWpaUUhTY2J4TmF5Y0Jmb3NOd1JPWUovVTVJSgpBck9JeTVSNy95 NlB6UHdFK2lQRUlZWEEvbzd0ei93YW1Pb2daV2NUOFp4SlZDL1VsU2tjeDVTaDd6MHhBa2JDCklT YnV5NFZ0RGhPcDArQkw4ZzRWYXNDWWYxeVlzOFNGZHpjS3c3NkZnWU00L1FtcjhWUnV1a1lMMjFT YkhmV1YKTytXM0cvTVpFNU5MVjc5eDMzZnNQTmtmdllpSmF2MnhGMUlZS2RlNGR5SkdHM0hLZDV1 UzNPR284eFU2cmhwUgpXVWZMamxkaGVVdlA0WlZPMEhjR1BKVEhpYTFLOGJGQjFxZndrWUR1ZXRC aHBJQ0hrK21icFdQOTJPZkJ2dEpvCjIvZW4xdHFjaUlMalp6LytlUlJSYTlrVG5laFZXZz09Cj0z ZjkxCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo= --===============0856864945193865002==--