Message-ID: <CAAFQd5B8UVk3n7m+MV3t68vrDhtd9Hi_CnuYS-4QFaVdByOTyA@mail.gmail.com>
Date:   Thu, 31 May 2018 16:06:05 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Stanimir Varbanov <stanimir.varbanov@...aro.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        vgarodia@...eaurora.org
Subject: Re: [PATCH v2 12/29] venus: add common capability parser

On Thu, May 31, 2018 at 1:21 AM Stanimir Varbanov
<stanimir.varbanov@...aro.org> wrote:
>
> Hi Tomasz,
>
> On 05/24/2018 05:16 PM, Tomasz Figa wrote:
> > Hi Stanimir,
> >
> > On Tue, May 15, 2018 at 5:08 PM Stanimir Varbanov <
> > [snip]
> >> diff --git a/drivers/media/platform/qcom/venus/core.h
> > b/drivers/media/platform/qcom/venus/core.h
> >> index b5b9a84e9155..fe2d2b9e8af8 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -57,6 +57,29 @@ struct venus_format {
> >>           u32 type;
> >>    };
> >
> >> +#define MAX_PLANES             4
> >
> > We have VIDEO_MAX_PLANES (== 8) already.
>
> yes, but venus has maximum of 4
>

Generally we tend to avoid inventing new constants and so you can see
that many drivers just use VIDEO_MAX_PLANES. I can also see drivers
that don't, so I guess we can keep it as is.

> >>    #define IS_V1(core)    ((core)->res->hfi_version == HFI_VERSION_1XX)
> >> @@ -322,4 +320,18 @@ static inline void *to_hfi_priv(struct venus_core
> > *core)
> >>           return core->priv;
> >>    }
> >
> >> +static inline struct venus_caps *
> >
> > I'd leave the decision whether to inline this or not to the compiler.
> > (Although these days the "inline" keyword is just a hint anyway... but
> > still just wasted bytes in kernel's git repo.)
>
> I just followed the other code examples in the kernel and in venus. If
> you insist I can drop 'inline'.

https://www.kernel.org/doc/html/latest/process/coding-style.html#the-inline-disease

> >> +static void for_each_codec(struct venus_caps *caps, unsigned int
> > caps_num,
> >> +                          u32 codecs, u32 domain, func cb, void *data,
> >> +                          unsigned int size)
> >> +{
> >> +       struct venus_caps *cap;
> >> +       unsigned int i;
> >> +
> >> +       for (i = 0; i < caps_num; i++) {
> >> +               cap = &caps[i];
> >> +               if (cap->valid && cap->domain == domain)
> >
> > Is there any need to check cap->domain == domain? We could just skip if
> > cap->valid.
>
> yes, we need to check the domain because we can have the same codec for
> both domains decoder and encoder but with different capabilities.
>

Sorry, I guess my comment wasn't clear. The second if below was
already checking the domain.

> >
> > If we want to shorten the code, we could even do (cap->valid || cap->domain
> > != domain) and remove domain check from the if below.
> >
> >> +                       continue;
> >> +               if (cap->codec & codecs && cap->domain == domain)

Here ^

But generally, if we consider second part of my comment, the problem
would disappear.

> >> +                       cb(cap, data, size);
> >> +       }
> >> +}
[snip]
> >> +static void parse_profile_level(u32 codecs, u32 domain, void *data)
> >> +{
> >> +       struct hfi_profile_level_supported *pl = data;
> >> +       struct hfi_profile_level *proflevel = pl->profile_level;
> >> +       u32 count = pl->profile_count;
> >> +
> >> +       if (count > HFI_MAX_PROFILE_COUNT)
> >> +               return;
> >> +
> >> +       while (count) {
> >> +               proflevel = (void *)proflevel + sizeof(*proflevel);
> >
> > Isn’t this just ++proflevel?
>
> yes
>
> >
> >> +               count--;
> >> +       }
> >
> > Am I missing something or this function doesn’t to do anything?
>
> yes, currently it is not used. I'll update it.
>

I'd say we should just remove it for now and add it only when it is
actually needed for something.

> >
> >> +}
> >> +
> >> +static void fill_caps(struct venus_caps *cap, void *data, unsigned int
> > num)
> >> +{
> >> +       struct hfi_capability *caps = data;
> >> +       unsigned int i;
> >> +
> >
> > Should we have some check to avoid overflowing cap->caps[]?
>
> No, we checked that below 'num_caps > MAX_CAP_ENTRIES'
>

Ack.

> >> +static void parse_raw_formats(struct venus_core *core, struct venus_inst
> > *inst,
> >> +                             u32 codecs, u32 domain, void *data)
> >> +{
> >> +       struct hfi_uncompressed_format_supported *fmt = data;
> >> +       struct hfi_uncompressed_plane_info *pinfo = fmt->format_info;
> >> +       struct hfi_uncompressed_plane_constraints *constr;
> >> +       u32 entries = fmt->format_entries;
> >> +       u32 num_planes;
> >> +       struct raw_formats rfmts[MAX_FMT_ENTRIES] = {};
> >> +       unsigned int i = 0;
> >> +
> >> +       while (entries) {
> >> +               num_planes = pinfo->num_planes;
> >> +
> >> +               rfmts[i].fmt = pinfo->format;
> >> +               rfmts[i].buftype = fmt->buffer_type;
> >> +               i++;
> >> +
> >> +               if (pinfo->num_planes > MAX_PLANES)
> >> +                       break;
> >> +
> >> +               constr = pinfo->plane_format;
> >> +
> >> +               while (pinfo->num_planes) {
> >> +                       constr = (void *)constr + sizeof(*constr);
> >
> > ++constr?
> >
> >> +                       pinfo->num_planes--;
> >> +               }
> >
> > What is this loop supposed to do?
>
> It is a leftover for constraints per format and plane. Currently we
> don't use them, or at least the values returned by the firmware.
>

I guess it means we can remove it. :)

> >
> >> +
> >> +               pinfo = (void *)pinfo + sizeof(*constr) * num_planes +
> >> +                       2 * sizeof(u32);
> >> +               entries--;
> >> +       }
> >> +
> >> +       for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
> >> +                      fill_raw_fmts, rfmts, i);
> >> +}
> > [snip]
> >> +static void parser_fini(struct venus_core *core, struct venus_inst *inst,
> >> +                       u32 codecs, u32 domain)
> >> +{
> >> +       struct venus_caps *caps = core->caps;
> >> +       struct venus_caps *cap;
> >> +       u32 dom;
> >> +       unsigned int i;
> >> +
> >> +       if (core->res->hfi_version != HFI_VERSION_1XX)
> >> +               return;
> >
> > Hmm, if the code below is executed only for 1XX, who will set cap->valid to
> > true for newer versions?
>
> cap::valid is used only for v1xx. Will add a comment in the structure.
>

Yes, please.

> >
> >> +
> >> +       if (!inst)
> >> +               return;
> >> +
> >> +       dom = inst->session_type;
> >> +
> >> +       for (i = 0; i < MAX_CODEC_NUM; i++) {
> >> +               cap = &caps[i];
> >> +               if (cap->codec & codecs && cap->domain == dom)
> >> +                       cap->valid = true;
> >> +       }
> >> +}
> >> +
> >> +u32 hfi_parser(struct venus_core *core, struct venus_inst *inst,
> >> +              u32 num_properties, void *buf, u32 size)
> >> +{
> >> +       unsigned int words_count = size >> 2;
> >> +       u32 *word = buf, *data, codecs = 0, domain = 0;
> >> +
> >> +       if (size % 4)
> >> +               return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> >> +
> >> +       parser_init(core, inst, &codecs, &domain);
> >> +
> >> +       while (words_count) {
> >> +               data = word + 1;
> >> +
> >> +               switch (*word) {
> >> +               case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
> >> +                       parse_codecs(core, data);
> >> +                       init_codecs_vcaps(core);
> >> +                       break;
> >> +               case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
> >> +                       parse_max_sessions(core, data);
> >> +                       break;
> >> +               case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
> >> +                       parse_codecs_mask(&codecs, &domain, data);
> >> +                       break;
> >> +               case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
> >> +                       parse_raw_formats(core, inst, codecs, domain,
> > data);
> >> +                       break;
> >> +               case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
> >> +                       parse_caps(core, inst, codecs, domain, data);
> >> +                       break;
> >> +               case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
> >> +                       parse_profile_level(codecs, domain, data);
> >> +                       break;
> >> +               case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
> >> +                       parse_alloc_mode(core, inst, codecs, domain,
> > data);
> >> +                       break;
> >> +               default:
> >
> > Should we perhaps print something to let us know that something
> > unrecognized was reported? (Or it is expected that something unrecognized
> > is there?)
>
> The default case will be very loaded with the data of the structures, so
> I don't think a print is a good idea.
>

Ack.

> >
> >> +                       break;
> >> +               }
> >> +
> >> +               word++;
> >> +               words_count--;
> >
> > If data is at |word + 1|, shouldn’t we increment |word| by |1 + |data
> > size||?
>
> yes, that could be possible but the firmware packets are with variable
> data length and don't want to make the code so complex.
>
> The idea is to search for HFI_PROPERTY_PARAM* key numbers. Yes it is not
> optimal but this enumeration is happen only once during driver probe.
>

Hmm, do we have a guarantee that we will never find a value that
matches HFI_PROPERTY_PARAM*, but would be actually just some data
inside the payload?

Best regards,
Tomasz

Powered by blists - more mailing lists