Message ID | 20240402000033.4007-2-laurent.pinchart@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series | Support for the generic line-based metadata support | expand |
Hi Sakari, Thank you for the patch. On Tue, Apr 02, 2024 at 03:00:26AM +0300, Laurent Pinchart wrote: > From: Sakari Ailus <sakari.ailus@linux.intel.com> > > Print the MUST_CONNECT pad flag for each pad. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > utils/media-ctl/media-ctl.c | 50 +++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 22 deletions(-) > > diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c > index 2081f111f2db..1b40552253f1 100644 > --- a/utils/media-ctl/media-ctl.c > +++ b/utils/media-ctl/media-ctl.c > @@ -368,26 +368,6 @@ static const char *media_entity_subtype_to_string(unsigned type) > } > } > > -static const char *media_pad_type_to_string(unsigned flag) > -{ > - static const struct { > - __u32 flag; > - const char *name; > - } flags[] = { > - { MEDIA_PAD_FL_SINK, "Sink" }, > - { MEDIA_PAD_FL_SOURCE, "Source" }, > - }; > - > - unsigned int i; > - > - for (i = 0; i < ARRAY_SIZE(flags); i++) { > - if (flags[i].flag & flag) > - return flags[i].name; > - } > - > - return "Unknown"; > -} > - > static void media_print_topology_dot(struct media_device *media) > { > unsigned int nents = media_get_entities_count(media); > @@ -525,6 +505,25 @@ static void media_print_pad_text(struct media_entity *entity, > v4l2_subdev_print_subdev_dv(entity); > } > > +static unsigned int weight(uint32_t word) > +{ > + unsigned int w = 0, i; > + > + for (i = 0; i < sizeof(word) << 3; i++, word >>= 1) > + w += word & 1U; > + > + return w; > +} > + > +static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags) > +{ > + static const char *empty = "", *comma = ", "; > + if (!(flag & flags)) > + return empty; > + > + return weight(prev_flags & flags) ? comma : empty; Unless I'm mistaken, we can write this return prev_flags & flags ? comma : empty; and drop the weight function. > +} > + > static void media_print_topology_text_entity(struct media_device *media, > struct media_entity *entity) > { > @@ -567,8 +566,15 @@ static void media_print_topology_text_entity(struct media_device *media, > for (j = 0; j < info->pads; j++) { > const struct media_pad *pad = media_entity_get_pad(entity, j); > > - printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags)); > - > + printf("\tpad%u: %s%s%s%s%s\n", j, > + pad->flags & MEDIA_PAD_FL_SINK ? "Sink" : "", > + comma(MEDIA_PAD_FL_SOURCE, MEDIA_PAD_FL_SINK, > + pad->flags), > + pad->flags & MEDIA_PAD_FL_SOURCE ? "Source" : "", > + comma(MEDIA_PAD_FL_MUST_CONNECT, > + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE, > + pad->flags), > + pad->flags & MEDIA_PAD_FL_MUST_CONNECT ? "Must connect" : ""); To be honest, this looks overly complicated. How about printing the flags with a loop ? > media_print_pad_text(entity, pad, routes, num_routes); > > for (k = 0; k < num_links; k++) {
On 03/04/2024 11:40, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Apr 02, 2024 at 03:00:26AM +0300, Laurent Pinchart wrote: >> From: Sakari Ailus <sakari.ailus@linux.intel.com> >> >> Print the MUST_CONNECT pad flag for each pad. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> utils/media-ctl/media-ctl.c | 50 +++++++++++++++++++++---------------- >> 1 file changed, 28 insertions(+), 22 deletions(-) >> >> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c >> index 2081f111f2db..1b40552253f1 100644 >> --- a/utils/media-ctl/media-ctl.c >> +++ b/utils/media-ctl/media-ctl.c >> @@ -368,26 +368,6 @@ static const char *media_entity_subtype_to_string(unsigned type) >> } >> } >> >> -static const char *media_pad_type_to_string(unsigned flag) >> -{ >> - static const struct { >> - __u32 flag; >> - const char *name; >> - } flags[] = { >> - { MEDIA_PAD_FL_SINK, "Sink" }, >> - { MEDIA_PAD_FL_SOURCE, "Source" }, >> - }; >> - >> - unsigned int i; >> - >> - for (i = 0; i < ARRAY_SIZE(flags); i++) { >> - if (flags[i].flag & flag) >> - return flags[i].name; >> - } >> - >> - return "Unknown"; >> -} >> - >> static void media_print_topology_dot(struct media_device *media) >> { >> unsigned int nents = media_get_entities_count(media); >> @@ -525,6 +505,25 @@ static void media_print_pad_text(struct media_entity *entity, >> v4l2_subdev_print_subdev_dv(entity); >> } >> >> +static unsigned int weight(uint32_t word) >> +{ >> + unsigned int w = 0, i; >> + >> + for (i = 0; i < sizeof(word) << 3; i++, word >>= 1) >> + w += word & 1U; >> + >> + return w; >> +} >> + >> +static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags) >> +{ >> + static const char *empty = "", *comma = ", "; >> + if (!(flag & flags)) >> + return empty; >> + >> + return weight(prev_flags & flags) ? comma : empty; > > Unless I'm mistaken, we can write this > > return prev_flags & flags ? comma : empty; > > and drop the weight function. > >> +} >> + >> static void media_print_topology_text_entity(struct media_device *media, >> struct media_entity *entity) >> { >> @@ -567,8 +566,15 @@ static void media_print_topology_text_entity(struct media_device *media, >> for (j = 0; j < info->pads; j++) { >> const struct media_pad *pad = media_entity_get_pad(entity, j); >> >> - printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags)); >> - >> + printf("\tpad%u: %s%s%s%s%s\n", j, >> + pad->flags & MEDIA_PAD_FL_SINK ? "Sink" : "", >> + comma(MEDIA_PAD_FL_SOURCE, MEDIA_PAD_FL_SINK, >> + pad->flags), >> + pad->flags & MEDIA_PAD_FL_SOURCE ? "Source" : "", >> + comma(MEDIA_PAD_FL_MUST_CONNECT, >> + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE, >> + pad->flags), >> + pad->flags & MEDIA_PAD_FL_MUST_CONNECT ? "Must connect" : ""); > > To be honest, this looks overly complicated. How about printing the > flags with a loop ? I was just about to reply that this looks a bit too "smart" to me... I'd prefer just a simple loop here for readability's and maintainability's sake, even if it's not as optimal. Tomi
On 03/04/2024 10:43, Tomi Valkeinen wrote: > On 03/04/2024 11:40, Laurent Pinchart wrote: >> Hi Sakari, >> >> Thank you for the patch. >> >> On Tue, Apr 02, 2024 at 03:00:26AM +0300, Laurent Pinchart wrote: >>> From: Sakari Ailus <sakari.ailus@linux.intel.com> >>> >>> Print the MUST_CONNECT pad flag for each pad. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> utils/media-ctl/media-ctl.c | 50 +++++++++++++++++++++---------------- >>> 1 file changed, 28 insertions(+), 22 deletions(-) >>> >>> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c >>> index 2081f111f2db..1b40552253f1 100644 >>> --- a/utils/media-ctl/media-ctl.c >>> +++ b/utils/media-ctl/media-ctl.c >>> @@ -368,26 +368,6 @@ static const char *media_entity_subtype_to_string(unsigned type) >>> } >>> } >>> -static const char *media_pad_type_to_string(unsigned flag) >>> -{ >>> - static const struct { >>> - __u32 flag; >>> - const char *name; >>> - } flags[] = { >>> - { MEDIA_PAD_FL_SINK, "Sink" }, >>> - { MEDIA_PAD_FL_SOURCE, "Source" }, >>> - }; >>> - >>> - unsigned int i; >>> - >>> - for (i = 0; i < ARRAY_SIZE(flags); i++) { >>> - if (flags[i].flag & flag) >>> - return flags[i].name; >>> - } >>> - >>> - return "Unknown"; >>> -} >>> - >>> static void media_print_topology_dot(struct media_device *media) >>> { >>> unsigned int nents = media_get_entities_count(media); >>> @@ -525,6 +505,25 @@ static void media_print_pad_text(struct media_entity *entity, >>> v4l2_subdev_print_subdev_dv(entity); >>> } >>> +static unsigned int weight(uint32_t word) >>> +{ >>> + unsigned int w = 0, i; >>> + >>> + for (i = 0; i < sizeof(word) << 3; i++, word >>= 1) >>> + w += word & 1U; >>> + >>> + return w; >>> +} >>> + >>> +static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags) >>> +{ >>> + static const char *empty = "", *comma = ", "; >>> + if (!(flag & flags)) >>> + return empty; >>> + >>> + return weight(prev_flags & flags) ? comma : empty; >> >> Unless I'm mistaken, we can write this >> >> return prev_flags & flags ? comma : empty; >> >> and drop the weight function. >> >>> +} >>> + >>> static void media_print_topology_text_entity(struct media_device *media, >>> struct media_entity *entity) >>> { >>> @@ -567,8 +566,15 @@ static void media_print_topology_text_entity(struct media_device *media, >>> for (j = 0; j < info->pads; j++) { >>> const struct media_pad *pad = media_entity_get_pad(entity, j); >>> - printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags)); >>> - >>> + printf("\tpad%u: %s%s%s%s%s\n", j, >>> + pad->flags & MEDIA_PAD_FL_SINK ? "Sink" : "", >>> + comma(MEDIA_PAD_FL_SOURCE, MEDIA_PAD_FL_SINK, >>> + pad->flags), >>> + pad->flags & MEDIA_PAD_FL_SOURCE ? "Source" : "", >>> + comma(MEDIA_PAD_FL_MUST_CONNECT, >>> + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE, >>> + pad->flags), >>> + pad->flags & MEDIA_PAD_FL_MUST_CONNECT ? "Must connect" : ""); >> >> To be honest, this looks overly complicated. How about printing the >> flags with a loop ? > > I was just about to reply that this looks a bit too "smart" to me... I'd prefer just a simple loop here for readability's and maintainability's sake, even if it's not as optimal. Same comment from me :-) Regards, Hans
On Thu, Apr 04, 2024 at 08:17:09PM +0000, Sakari Ailus wrote: > On Wed, Apr 03, 2024 at 10:56:55AM +0200, Hans Verkuil wrote: > > On 03/04/2024 10:43, Tomi Valkeinen wrote: > > > On 03/04/2024 11:40, Laurent Pinchart wrote: > > >> On Tue, Apr 02, 2024 at 03:00:26AM +0300, Laurent Pinchart wrote: > > >>> From: Sakari Ailus <sakari.ailus@linux.intel.com> > > >>> > > >>> Print the MUST_CONNECT pad flag for each pad. > > >>> > > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > >>> --- > > >>> utils/media-ctl/media-ctl.c | 50 +++++++++++++++++++++---------------- > > >>> 1 file changed, 28 insertions(+), 22 deletions(-) > > >>> > > >>> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c > > >>> index 2081f111f2db..1b40552253f1 100644 > > >>> --- a/utils/media-ctl/media-ctl.c > > >>> +++ b/utils/media-ctl/media-ctl.c > > >>> @@ -368,26 +368,6 @@ static const char *media_entity_subtype_to_string(unsigned type) > > >>> } > > >>> } > > >>> -static const char *media_pad_type_to_string(unsigned flag) > > >>> -{ > > >>> - static const struct { > > >>> - __u32 flag; > > >>> - const char *name; > > >>> - } flags[] = { > > >>> - { MEDIA_PAD_FL_SINK, "Sink" }, > > >>> - { MEDIA_PAD_FL_SOURCE, "Source" }, > > >>> - }; > > >>> - > > >>> - unsigned int i; > > >>> - > > >>> - for (i = 0; i < ARRAY_SIZE(flags); i++) { > > >>> - if (flags[i].flag & flag) > > >>> - return flags[i].name; > > >>> - } > > >>> - > > >>> - return "Unknown"; > > >>> -} > > >>> - > > >>> static void media_print_topology_dot(struct media_device *media) > > >>> { > > >>> unsigned int nents = media_get_entities_count(media); > > >>> @@ -525,6 +505,25 @@ static void media_print_pad_text(struct media_entity *entity, > > >>> v4l2_subdev_print_subdev_dv(entity); > > >>> } > > >>> +static unsigned int weight(uint32_t word) > > >>> +{ > > >>> + unsigned int w = 0, i; > > >>> + > > >>> + for (i = 0; i < sizeof(word) << 3; i++, word >>= 1) > > >>> + w += word & 1U; > > >>> + > > >>> + return w; > > >>> +} > > >>> + > > >>> +static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags) > > >>> +{ > > >>> + static const char *empty = "", *comma = ", "; > > >>> + if (!(flag & flags)) > > >>> + return empty; > > >>> + > > >>> + return weight(prev_flags & flags) ? comma : empty; > > >> > > >> Unless I'm mistaken, we can write this > > >> > > >> return prev_flags & flags ? comma : empty; > > >> > > >> and drop the weight function. > > Correct. An earlier version of the patch used it and I forgot to remove it. > > It should be possible to write this as: > > static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags) > { > return flag & flags && prev_flags & flags ? ", " : ""; > } > > This nicely demonstrates C operator precedence. > > > >> > > >>> +} > > >>> + > > >>> static void media_print_topology_text_entity(struct media_device *media, > > >>> struct media_entity *entity) > > >>> { > > >>> @@ -567,8 +566,15 @@ static void media_print_topology_text_entity(struct media_device *media, > > >>> for (j = 0; j < info->pads; j++) { > > >>> const struct media_pad *pad = media_entity_get_pad(entity, j); > > >>> - printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags)); > > >>> - > > >>> + printf("\tpad%u: %s%s%s%s%s\n", j, > > >>> + pad->flags & MEDIA_PAD_FL_SINK ? "Sink" : "", > > >>> + comma(MEDIA_PAD_FL_SOURCE, MEDIA_PAD_FL_SINK, > > >>> + pad->flags), > > >>> + pad->flags & MEDIA_PAD_FL_SOURCE ? "Source" : "", > > >>> + comma(MEDIA_PAD_FL_MUST_CONNECT, > > >>> + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE, > > >>> + pad->flags), > > >>> + pad->flags & MEDIA_PAD_FL_MUST_CONNECT ? "Must connect" : ""); > > >> > > >> To be honest, this looks overly complicated. How about printing the > > >> flags with a loop ? > > > > > > I was just about to reply that this looks a bit too "smart" to me... I'd prefer just a simple loop here for readability's and maintainability's sake, even if it's not as optimal. > > > > Same comment from me :-) > > The above gets it done as a single printf call. Perhaps it doesn't matter > much if it doesn't though, this isn't printk. Still do note that checking > whether the commas will be printed isn't trivial so replacing this with a > loop isn't necessarily making the code notably simpler. See https://lore.kernel.org/linux-media/20240404220312.8019-1-laurent.pinchart@ideasonboard.com
diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c index 2081f111f2db..1b40552253f1 100644 --- a/utils/media-ctl/media-ctl.c +++ b/utils/media-ctl/media-ctl.c @@ -368,26 +368,6 @@ static const char *media_entity_subtype_to_string(unsigned type) } } -static const char *media_pad_type_to_string(unsigned flag) -{ - static const struct { - __u32 flag; - const char *name; - } flags[] = { - { MEDIA_PAD_FL_SINK, "Sink" }, - { MEDIA_PAD_FL_SOURCE, "Source" }, - }; - - unsigned int i; - - for (i = 0; i < ARRAY_SIZE(flags); i++) { - if (flags[i].flag & flag) - return flags[i].name; - } - - return "Unknown"; -} - static void media_print_topology_dot(struct media_device *media) { unsigned int nents = media_get_entities_count(media); @@ -525,6 +505,25 @@ static void media_print_pad_text(struct media_entity *entity, v4l2_subdev_print_subdev_dv(entity); } +static unsigned int weight(uint32_t word) +{ + unsigned int w = 0, i; + + for (i = 0; i < sizeof(word) << 3; i++, word >>= 1) + w += word & 1U; + + return w; +} + +static const char *comma(uint32_t flag, uint32_t prev_flags, uint32_t flags) +{ + static const char *empty = "", *comma = ", "; + if (!(flag & flags)) + return empty; + + return weight(prev_flags & flags) ? comma : empty; +} + static void media_print_topology_text_entity(struct media_device *media, struct media_entity *entity) { @@ -567,8 +566,15 @@ static void media_print_topology_text_entity(struct media_device *media, for (j = 0; j < info->pads; j++) { const struct media_pad *pad = media_entity_get_pad(entity, j); - printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags)); - + printf("\tpad%u: %s%s%s%s%s\n", j, + pad->flags & MEDIA_PAD_FL_SINK ? "Sink" : "", + comma(MEDIA_PAD_FL_SOURCE, MEDIA_PAD_FL_SINK, + pad->flags), + pad->flags & MEDIA_PAD_FL_SOURCE ? "Source" : "", + comma(MEDIA_PAD_FL_MUST_CONNECT, + MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE, + pad->flags), + pad->flags & MEDIA_PAD_FL_MUST_CONNECT ? "Must connect" : ""); media_print_pad_text(entity, pad, routes, num_routes); for (k = 0; k < num_links; k++) {