linux-kernel - Re: [PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <590e52a2-d7ea-4b86-b1f2-a6875118653c@xs4all.nl>
Date: Tue, 30 Jul 2024 09:15:22 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Sebastian Fricke <sebastian.fricke@...labora.com>,
 Benjamin Gaignard <benjamin.gaignard@...labora.com>
Cc: mchehab@...nel.org, ezequiel@...guardiasur.com.ar,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v5 2/3] media: test-drivers: Use V4L2_FMT_FLAG_ENUM_ALL
 flag

On 23/07/2024 11:49, Sebastian Fricke wrote:
> Hey Benjamin,
> 
> On 22.07.2024 17:05, Benjamin Gaignard wrote:
>> Since V4L2_FMT_FLAG_ENUM_ALL flag mostly targeting stateless
> 
> s/Since/Since the/
> s/targeting/targets/
> 
>> decoder pixel formats enumeration, update vicodec visl test
> 
> s/pixel formats/pixel-format/
> 
>> drivers to use it.
> 
> s/drivers/driver/
> 
> The rest below basically just strips the flag from every use-case of the
> index, before using it.
> 
> I wonder couldn't we implement a macro for this, as I believe this will
> have to be done in a lot of places, something like:
> 
> /*
>  * Drivers can support an enumeration of all formats, by ORing the
>  * V4L2_FMT_FLAG_ENUM_ALL flag into the index field.
>  * In order to use the index field, strip the flag first.
>  *
>  * See vidioc-enum-fmt.rst documentation for more details.
>  */
> #define STRIP_ENUM_ALL_FLAG(index) \
>   index & ~V4L2_FMT_FLAG_ENUM_ALL
> 
> And then use it like this:
> 
> u32 index = STRIP_ENUM_ALL_FLAG(f->index);

There is really no advantage to that. It is much better to be explicit
about it rather than hiding it in a define.

Regards,

	Hans

> 
> If we define this in a common header, then every driver can easily
> utilize it and you have built-in documentation.
> 
> What do you think?
> 
> Regards,
> Sebastian
> 
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
>> ---
>> drivers/media/test-drivers/vicodec/vicodec-core.c |  7 ++++---
>> drivers/media/test-drivers/visl/visl-video.c      | 11 +++++++----
>> 2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
>> index 3e011fe62ae1..1b4cd8ddd7c2 100644
>> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
>> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
>> @@ -706,6 +706,7 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>>             bool is_out)
>> {
>>     bool is_uncomp = (ctx->is_enc && is_out) || (!ctx->is_enc && !is_out);
>> +    u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>>
>>     if (V4L2_TYPE_IS_MULTIPLANAR(f->type) && !multiplanar)
>>         return -EINVAL;
>> @@ -718,18 +719,18 @@ static int enum_fmt(struct v4l2_fmtdesc *f, struct vicodec_ctx *ctx,
>>
>>         if (ctx->is_enc ||
>>             !vb2_is_streaming(&ctx->fh.m2m_ctx->cap_q_ctx.q))
>> -            info = v4l2_fwht_get_pixfmt(f->index);
>> +            info = v4l2_fwht_get_pixfmt(index);
>>         else
>>             info = v4l2_fwht_find_nth_fmt(info->width_div,
>>                              info->height_div,
>>                              info->components_num,
>>                              info->pixenc,
>> -                             f->index);
>> +                             index);
>>         if (!info)
>>             return -EINVAL;
>>         f->pixelformat = info->id;
>>     } else {
>> -        if (f->index)
>> +        if (index)
>>             return -EINVAL;
>>         f->pixelformat = ctx->is_stateless ?
>>             V4L2_PIX_FMT_FWHT_STATELESS : V4L2_PIX_FMT_FWHT;
>> diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
>> index f8d970319764..c5f3e13b4198 100644
>> --- a/drivers/media/test-drivers/visl/visl-video.c
>> +++ b/drivers/media/test-drivers/visl/visl-video.c
>> @@ -341,21 +341,24 @@ static int visl_enum_fmt_vid_cap(struct file *file, void *priv,
>>                  struct v4l2_fmtdesc *f)
>> {
>>     struct visl_ctx *ctx = visl_file_to_ctx(file);
>> +    u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>>
>> -    if (f->index >= ctx->coded_format_desc->num_decoded_fmts)
>> +    if (index >= ctx->coded_format_desc->num_decoded_fmts)
>>         return -EINVAL;
>>
>> -    f->pixelformat = ctx->coded_format_desc->decoded_fmts[f->index];
>> +    f->pixelformat = ctx->coded_format_desc->decoded_fmts[index];
>>     return 0;
>> }
>>
>> static int visl_enum_fmt_vid_out(struct file *file, void *priv,
>>                  struct v4l2_fmtdesc *f)
>> {
>> -    if (f->index >= ARRAY_SIZE(visl_coded_fmts))
>> +    u32 index = f->index & ~V4L2_FMT_FLAG_ENUM_ALL;
>> +
>> +    if (index >= ARRAY_SIZE(visl_coded_fmts))
>>         return -EINVAL;
>>
>> -    f->pixelformat = visl_coded_fmts[f->index].pixelformat;
>> +    f->pixelformat = visl_coded_fmts[index].pixelformat;
>>     return 0;
>> }
>>
>> -- 
>> 2.43.0
>>
>> _______________________________________________
>> Kernel mailing list -- kernel@...lman.collabora.com
>> To unsubscribe send an email to kernel-leave@...lman.collabora.com
>> This list is managed by https://mailman.collabora.com
>