| Message ID | 20230117122244.2546597-8-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand |
Hi Andy, On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: > > For all _DSD properties, skip going through the MIPI DisCo for Imaging > > property name substitution table if the property doesn't have "mipi-img-" > > prefix. > > ... > > > +#define MIPI_IMG_PREFIX "mipi-img-" > > I don't think this is good for readability. It should be used at least below, where it is referred twice. I'm open to removing this from the table though. > > ... > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > str_has_prefix() str_has_prefix() calls strlen() on prefix on every call. sizeof() will generate much less code --- it's just a number. > > > + return;
On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: ... > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > str_has_prefix() > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > generate much less code --- it's just a number. Have you tried that? Because the strlen() over const string literals will be optimized away on compilation time. > > > + return;
On Thu, Jan 19, 2023 at 06:09:26PM +0200, Andy Shevchenko wrote: > On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: ... > > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > > > str_has_prefix() > > > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > > generate much less code --- it's just a number. > > Have you tried that? Because the strlen() over const string literals will be > optimized away on compilation time. Probably that's the reason behind __always_inline for that function. > > > > + return;
On Thu, Jan 19, 2023 at 06:09:25PM +0200, Andy Shevchenko wrote: > On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: > > ... > > > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > > > str_has_prefix() > > > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > > generate much less code --- it's just a number. > > Have you tried that? Because the strlen() over const string literals will be > optimized away on compilation time. Actually not. There seem to be an implementation of strlen() in include/linux/fortify-string.h that would seem to be capable of doing that. However its use is conditional to kernel configuration.
On Thu, Jan 19, 2023 at 06:11:11PM +0200, Andy Shevchenko wrote: > On Thu, Jan 19, 2023 at 06:09:26PM +0200, Andy Shevchenko wrote: > > On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > > > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: > > ... > > > > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > > > > > str_has_prefix() > > > > > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > > > generate much less code --- it's just a number. > > > > Have you tried that? Because the strlen() over const string literals will be > > optimized away on compilation time. > > Probably that's the reason behind __always_inline for that function. For str_has_prefix() the reason probably is that inlining that function generates less code than when not doing so.
On Fri, Jan 20, 2023 at 12:07:41PM +0000, Sakari Ailus wrote: > On Thu, Jan 19, 2023 at 06:11:11PM +0200, Andy Shevchenko wrote: > > On Thu, Jan 19, 2023 at 06:09:26PM +0200, Andy Shevchenko wrote: > > > On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > > > > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: ... > > > > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > > > > > > > str_has_prefix() > > > > > > > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > > > > generate much less code --- it's just a number. > > > > > > Have you tried that? Because the strlen() over const string literals will be > > > optimized away on compilation time. > > > > Probably that's the reason behind __always_inline for that function. > > For str_has_prefix() the reason probably is that inlining that function > generates less code than when not doing so. Yes and also allows to optimize strlen() away. So I suggest to use that function. If assembly is different (WRT strlen("...const literal...") case), I would like to know the exact configuration options and the code that makes a call to strlen().
On Fri, Jan 20, 2023 at 11:58:52AM +0000, Sakari Ailus wrote: > On Thu, Jan 19, 2023 at 06:09:25PM +0200, Andy Shevchenko wrote: > > On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > > > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: ... > > > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > > > > > str_has_prefix() > > > > > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > > > generate much less code --- it's just a number. > > > > Have you tried that? Because the strlen() over const string literals will be > > optimized away on compilation time. > > Actually not. There seem to be an implementation of strlen() in > include/linux/fortify-string.h that would seem to be capable of doing that. > However its use is conditional to kernel configuration. Ah, you missed probably the ability of the complier to find constant literals and replace the strlen() with plain number. You may play with godbolt and see how optimization (-O2) makes this happen.
On Fri, Jan 20, 2023 at 05:11:27PM +0200, Andy Shevchenko wrote: > On Fri, Jan 20, 2023 at 11:58:52AM +0000, Sakari Ailus wrote: > > On Thu, Jan 19, 2023 at 06:09:25PM +0200, Andy Shevchenko wrote: > > > On Thu, Jan 19, 2023 at 03:51:33PM +0000, Sakari Ailus wrote: > > > > On Tue, Jan 17, 2023 at 05:29:54PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Jan 17, 2023 at 02:22:43PM +0200, Sakari Ailus wrote: > > ... > > > > > > > + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, > > > > > > + sizeof(MIPI_IMG_PREFIX) - 1)) > > > > > > > > > > str_has_prefix() > > > > > > > > str_has_prefix() calls strlen() on prefix on every call. sizeof() will > > > > generate much less code --- it's just a number. > > > > > > Have you tried that? Because the strlen() over const string literals will be > > > optimized away on compilation time. > > > > Actually not. There seem to be an implementation of strlen() in > > include/linux/fortify-string.h that would seem to be capable of doing that. > > However its use is conditional to kernel configuration. > > Ah, you missed probably the ability of the complier to find constant literals > and replace the strlen() with plain number. It seems GCC does this if -foptimize-strlen (included in -O2) is given. Fair enough, I'll replace it with str_has_prefix() for v2.
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c index 2174e03a2eafd..840f565b1906f 100644 --- a/drivers/acpi/mipi.c +++ b/drivers/acpi/mipi.c @@ -688,16 +688,18 @@ void acpi_init_swnodes(struct acpi_device *device) device->fwnode.secondary = software_node_fwnode(ads->nodes); } +#define MIPI_IMG_PREFIX "mipi-img-" + static const struct mipi_disco_prop { const char *mipi_prop; const char *dt_prop; } mipi_disco_props[] = { - { "mipi-img-lens-focus", "lens-focus" }, - { "mipi-img-flash-leds", "flash-leds" }, - { "mipi-img-clock-frequency", "clock-frequency" }, - { "mipi-img-led-max-current", "led-max-microamp" }, - { "mipi-img-flash-max-current", "flash-max-microamp" }, - { "mipi-img-flash-max-timeout", "flash-max-timeout-us" }, + { MIPI_IMG_PREFIX "lens-focus", "lens-focus" }, + { MIPI_IMG_PREFIX "flash-leds", "flash-leds" }, + { MIPI_IMG_PREFIX "clock-frequency", "clock-frequency" }, + { MIPI_IMG_PREFIX "led-max-current", "led-max-microamp" }, + { MIPI_IMG_PREFIX "flash-max-current", "flash-max-microamp" }, + { MIPI_IMG_PREFIX "flash-max-timeout", "flash-max-timeout-us" }, }; /** @@ -713,6 +715,10 @@ void acpi_properties_prepare_mipi(union acpi_object *elements) { unsigned int i; + if (memcmp(elements[0].string.pointer, MIPI_IMG_PREFIX, + sizeof(MIPI_IMG_PREFIX) - 1)) + return; + /* Replace MIPI DisCo for Imaging property names with DT equivalents. */ for (i = 0; i < ARRAY_SIZE(mipi_disco_props); i++) { if (!strcmp(mipi_disco_props[i].mipi_prop,
For all _DSD properties, skip going through the MIPI DisCo for Imaging property name substitution table if the property doesn't have "mipi-img-" prefix. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/acpi/mipi.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)