diff mbox series

[v4l-utils,1/8] utils: media-ctl: Print MUST_CONNECT pad flags

Message ID 20240402000033.4007-2-laurent.pinchart@ideasonboard.com
State New
Headers show
Series Support for the generic line-based metadata support | expand

Commit Message

Laurent Pinchart April 2, 2024, midnight UTC 
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(-)

Comments

Laurent Pinchart April 3, 2024, 8:40 a.m. UTC | #1 
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++) {
Tomi Valkeinen April 3, 2024, 8:43 a.m. UTC | #2 
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
Hans Verkuil April 3, 2024, 8:56 a.m. UTC | #3 
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
Laurent Pinchart April 4, 2024, 10:04 p.m. UTC | #4 
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 mbox series

Patch

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++) {