lkml.org 
[lkml]   [2018]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v2 12/29] venus: add common capability parser
    Hi Stanimir,

    On Tue, May 15, 2018 at 5:08 PM Stanimir Varbanov <
    stanimir.varbanov@linaro.org> wrote:
    [snip]
    > diff --git a/drivers/media/platform/qcom/venus/core.c
    b/drivers/media/platform/qcom/venus/core.c
    > index 41eef376eb2d..381bfdd688db 100644
    > --- a/drivers/media/platform/qcom/venus/core.c
    > +++ b/drivers/media/platform/qcom/venus/core.c
    [snip]
    > +static int venus_enumerate_codecs(struct venus_core *core, u32 type)
    > +{
    [snip]
    > + for (i = 0; i < MAX_CODEC_NUM; i++) {
    > + codec = (1 << i) & codecs;
    > + if (!codec)
    > + continue;

    Could be simplified to

    for_each_set_bit(i, &codecs, MAX_CODEC_NUM) {

    after making codecs an unsigned long.

    [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.

    [snip]
    > @@ -224,22 +249,8 @@ struct venus_buffer {
    > * @priv: a private for HFI operations callbacks
    > * @session_type: the type of the session (decoder or encoder)
    > * @hprop: a union used as a holder by get property
    > - * @cap_width: width capability
    > - * @cap_height: height capability
    > - * @cap_mbs_per_frame: macroblocks per frame capability
    > - * @cap_mbs_per_sec: macroblocks per second capability
    > - * @cap_framerate: framerate capability
    > - * @cap_scale_x: horizontal scaling capability
    > - * @cap_scale_y: vertical scaling capability
    > - * @cap_bitrate: bitrate capability
    > - * @cap_hier_p: hier capability
    > - * @cap_ltr_count: LTR count capability
    > - * @cap_secure_output2_threshold: secure OUTPUT2 threshold capability
    > * @cap_bufs_mode_static: buffers allocation mode capability
    > * @cap_bufs_mode_dynamic: buffers allocation mode capability
    > - * @pl_count: count of supported profiles/levels
    > - * @pl: supported profiles/levels
    > - * @bufreq: holds buffer requirements
    > */
    > struct venus_inst {
    > struct list_head list;
    > @@ -276,6 +287,7 @@ struct venus_inst {
    > bool reconfig;
    > u32 reconfig_width;
    > u32 reconfig_height;
    > + u32 hfi_codec;

    Respective line not added to the kerneldoc above.

    > u32 sequence_cap;
    > u32 sequence_out;
    > struct v4l2_m2m_dev *m2m_dev;
    > @@ -287,22 +299,8 @@ struct venus_inst {
    > const struct hfi_inst_ops *ops;
    > u32 session_type;
    > union hfi_get_property hprop;
    > - struct hfi_capability cap_width;
    > - struct hfi_capability cap_height;
    > - struct hfi_capability cap_mbs_per_frame;
    > - struct hfi_capability cap_mbs_per_sec;
    > - struct hfi_capability cap_framerate;
    > - struct hfi_capability cap_scale_x;
    > - struct hfi_capability cap_scale_y;
    > - struct hfi_capability cap_bitrate;
    > - struct hfi_capability cap_hier_p;
    > - struct hfi_capability cap_ltr_count;
    > - struct hfi_capability cap_secure_output2_threshold;
    > bool cap_bufs_mode_static;
    > bool cap_bufs_mode_dynamic;
    > - unsigned int pl_count;
    > - struct hfi_profile_level pl[HFI_MAX_PROFILE_COUNT];
    > - struct hfi_buffer_requirements bufreq[HFI_BUFFER_TYPE_MAX];
    > };

    > #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.)

    > +venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
    > +{
    > + unsigned int c;
    > +
    > + for (c = 0; c < MAX_CODEC_NUM; c++) {

    Shouldn't we iterate up to core->codecs_count?
    > + if (core->caps[c].codec == codec &&
    > + core->caps[c].domain == domain)
    > + return &core->caps[c];
    > + }
    > +
    > + return NULL;
    > +}
    > +
    [snip]
    > + error = hfi_parser(core, inst, num_properties, &pkt->data[0],

    nit: pkt->data?

    [snip]
    > static void hfi_session_init_done(struct venus_core *core,
    > struct venus_inst *inst, void *packet)
    > {
    > struct hfi_msg_session_init_done_pkt *pkt = packet;
    > - unsigned int error;
    > + u32 rem_bytes, error;

    > error = pkt->error_type;
    > if (error != HFI_ERR_NONE)
    > @@ -745,8 +423,14 @@ static void hfi_session_init_done(struct venus_core
    *core,
    > if (core->res->hfi_version != HFI_VERSION_1XX)
    > goto done;

    > - error = init_done_read_prop(core, inst, pkt);
    > + rem_bytes = pkt->shdr.hdr.size - sizeof(*pkt) + sizeof(u32);
    > + if (!rem_bytes) {

    I’m not sure how likely it is to happen, but given that pkt->shdr.hdr.size
    seems to come from hardware, perhaps we should make rem_bytes signed and
    check for <= 0?

    > + error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
    > + goto done;
    > + }

    > + error = hfi_parser(core, inst, pkt->num_properties, &pkt->data[0],
    > + rem_bytes);

    nit: pkt->data?

    > done:
    > inst->error = error;
    > complete(&inst->done);
    > diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
    b/drivers/media/platform/qcom/venus/hfi_parser.c
    > new file mode 100644
    > index 000000000000..f9181d999b23
    > --- /dev/null
    > +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
    > @@ -0,0 +1,295 @@
    > +// SPDX-License-Identifier: GPL-2.0
    > +/*
    > + * Copyright (C) 2018 Linaro Ltd.
    > + *
    > + * Author: Stanimir Varbanov <stanimir.varbanov@linaro.org>
    > + */
    > +#include <linux/kernel.h>
    > +
    > +#include "core.h"
    > +#include "hfi_helper.h"
    > +#include "hfi_parser.h"
    > +
    > +typedef void (*func)(struct venus_caps *cap, void *data, unsigned int
    size);

    Should we perhaps make data const? (My understanding is that it comes from
    firmware packet.)

    > +
    > +static void init_codecs_vcaps(struct venus_core *core)
    > +{
    > + struct venus_caps *caps = core->caps;
    > + struct venus_caps *cap;
    > + unsigned int i;
    > +
    > + for (i = 0; i < 8 * sizeof(core->dec_codecs); i++) {
    > + if ((1 << i) & core->dec_codecs) {

    for_each_set_bit()?

    > + cap = &caps[core->codecs_count++];
    > + cap->codec = (1 << i) & core->dec_codecs;

    Any need to AND with core->dec_codecs? This code wouldn’t be executed if
    the bit wasn’t set in the first place.

    Also BIT(i).

    > + cap->domain = VIDC_SESSION_TYPE_DEC;
    > + cap->valid = false;
    > + }
    > + }
    > +
    > + for (i = 0; i < 8 * sizeof(core->enc_codecs); i++) {
    > + if ((1 << i) & core->enc_codecs) {

    Ditto.

    > + cap = &caps[core->codecs_count++];
    > + cap->codec = (1 << i) & core->enc_codecs;

    Ditto.

    > + cap->domain = VIDC_SESSION_TYPE_ENC;
    > + cap->valid = false;
    > + }
    > + }
    > +}
    > +
    > +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.

    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)
    > + cb(cap, data, size);
    > + }
    > +}
    > +
    > +static void fill_buf_mode(struct venus_caps *cap, void *data, unsigned
    int num)
    > +{
    > + u32 *type = data;
    > +
    > + if (*type == HFI_BUFFER_MODE_DYNAMIC)
    > + cap->cap_bufs_mode_dynamic = true;
    > +}
    > +
    > +static void parse_alloc_mode(struct venus_core *core, struct venus_inst
    *inst,
    > + u32 codecs, u32 domain, void *data)
    > +{
    > + struct hfi_buffer_alloc_mode_supported *mode = data;
    > + u32 num_entries = mode->num_entries;
    > + u32 *type;
    > +
    > + if (num_entries > 16)

    What is 16? We should have a macro for it.

    > + return;
    > +
    > + type = mode->data;
    > +
    > + while (num_entries--) {
    > + if (mode->buffer_type == HFI_BUFFER_OUTPUT ||
    > + mode->buffer_type == HFI_BUFFER_OUTPUT2) {
    > + if (*type == HFI_BUFFER_MODE_DYNAMIC && inst)
    > + inst->cap_bufs_mode_dynamic = true;
    > +
    > + for_each_codec(core->caps, ARRAY_SIZE(core->caps),
    > + codecs, domain, fill_buf_mode,
    type, 1);
    > + }
    > +
    > + type++;
    > + }
    > +}
    > +
    > +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?

    > + count--;
    > + }

    Am I missing something or this function doesn’t to do anything?

    > +}
    > +
    > +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[]?

    > + for (i = 0; i < num; i++)
    > + cap->caps[cap->num_caps++] = caps[i];
    > +}
    > +
    > +static void parse_caps(struct venus_core *core, struct venus_inst *inst,
    > + u32 codecs, u32 domain, void *data)
    > +{
    > + struct hfi_capabilities *caps = data;
    > + struct hfi_capability *cap = caps->data;
    > + u32 num_caps = caps->num_capabilities;
    > + struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
    > + unsigned int i = 0;
    > +
    > + if (num_caps > MAX_CAP_ENTRIES)
    > + return;
    > +
    > + while (num_caps) {
    > + caps_arr[i++] = *cap;
    > + cap = (void *)cap + sizeof(*cap);

    ++cap?

    > + num_caps--;
    > + }

    Hmm, isn’t the whole loop just

    memcpy(caps_arr, cap, num_caps * sizeof(*cap));

    ?

    > +
    > + for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
    > + fill_caps, caps_arr, i);
    > +}
    > +
    > +static void fill_raw_fmts(struct venus_caps *cap, void *fmts,
    > + unsigned int num_fmts)
    > +{
    > + struct raw_formats *formats = fmts;
    > + unsigned int i;
    > +
    > + for (i = 0; i < num_fmts; i++)
    > + cap->fmts[cap->num_fmts++] = formats[i];
    > +}
    > +
    > +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?

    > +
    > + 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?

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

    > + break;
    > + }
    > +
    > + word++;
    > + words_count--;

    If data is at |word + 1|, shouldn’t we increment |word| by |1 + |data
    size||?
    > + }
    > +
    > + parser_fini(core, inst, codecs, domain);
    > +
    > + return HFI_ERR_NONE;
    > +}
    > diff --git a/drivers/media/platform/qcom/venus/hfi_parser.h
    b/drivers/media/platform/qcom/venus/hfi_parser.h
    > new file mode 100644
    > index 000000000000..c484ac91a8e2
    > --- /dev/null
    > +++ b/drivers/media/platform/qcom/venus/hfi_parser.h
    > @@ -0,0 +1,45 @@
    > +/* SPDX-License-Identifier: GPL-2.0 */
    > +/* Copyright (C) 2018 Linaro Ltd. */
    > +#ifndef __VENUS_HFI_PARSER_H__
    > +#define __VENUS_HFI_PARSER_H__
    > +
    > +#include "core.h"
    > +
    > +u32 hfi_parser(struct venus_core *core, struct venus_inst *inst,
    > + u32 num_properties, void *buf, u32 size);
    > +
    > +static inline struct hfi_capability *get_cap(struct venus_inst *inst,
    u32 type)
    > +{
    > + struct venus_core *core = inst->core;
    > + struct venus_caps *caps;
    > + unsigned int i;
    > +
    > + caps = venus_caps_by_codec(core, inst->hfi_codec,
    inst->session_type);
    > + if (!caps)
    > + return ERR_PTR(-EINVAL);
    > +
    > + for (i = 0; i < MAX_CAP_ENTRIES; i++) {

    Shouldn’t this iterate up to caps->num_capabilities? Also, shouldn’t
    caps->caps[i]->valid be checked as well?

    > + if (caps->caps[i].capability_type == type)
    > + return &caps->caps[i];
    > + }
    > +
    > + return ERR_PTR(-EINVAL);
    > +}

    Best regards,
    Tomasz

    \
     
     \ /
      Last update: 2018-05-24 16:18    [W:4.065 / U:0.104 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site