| Message ID | 20180515075859.17217-1-stanimir.varbanov@linaro.org |
|---|---|
| Headers | show |
| Series | Venus updates | expand |
On Tue, May 15, 2018 at 5:14 PM Stanimir Varbanov < stanimir.varbanov@linaro.org> wrote: > HFI version 4xx can pass more properties in the sequence change > event, extend the event structure with them. > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/hfi.h | 9 ++++++ > drivers/media/platform/qcom/venus/hfi_msgs.c | 46 ++++++++++++++++++++++++++++ > 2 files changed, 55 insertions(+) > diff --git a/drivers/media/platform/qcom/venus/hfi.h b/drivers/media/platform/qcom/venus/hfi.h > index 5466b7d60dd0..21376d93170f 100644 > --- a/drivers/media/platform/qcom/venus/hfi.h > +++ b/drivers/media/platform/qcom/venus/hfi.h > @@ -74,6 +74,15 @@ struct hfi_event_data { > u32 tag; > u32 profile; > u32 level; nit; Could we add a comment saying that it showed in 4xx? [snip] > + case HFI_PROPERTY_CONFIG_VDEC_ENTROPY: > + data_ptr += sizeof(u32); > + entropy_mode = *(u32 *)data_ptr; > + event.entropy_mode = entropy_mode; Is the |entropy_mode| local variable necessary? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 15, 2018 at 5:12 PM Stanimir Varbanov < stanimir.varbanov@linaro.org> wrote: > Add AXI halt support for version 4xx by using venus wrapper > registers. > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/hfi_venus.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c > index 734ce11b0ed0..53546174aab8 100644 > --- a/drivers/media/platform/qcom/venus/hfi_venus.c > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c > @@ -532,6 +532,23 @@ static int venus_halt_axi(struct venus_hfi_device *hdev) > u32 val; > int ret; > + if (hdev->core->res->hfi_version == HFI_VERSION_4XX) { > + val = venus_readl(hdev, WRAPPER_CPU_AXI_HALT); > + val |= BIT(16); Can we have the bit defined? > + venus_writel(hdev, WRAPPER_CPU_AXI_HALT, val); > + > + ret = readl_poll_timeout(base + WRAPPER_CPU_AXI_HALT_STATUS, > + val, val & BIT(24), Ditto. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On 05/18/2018 06:14 PM, Tomasz Figa wrote: > On Tue, May 15, 2018 at 5:11 PM Stanimir Varbanov < > stanimir.varbanov@linaro.org> wrote: > >> This fixes the suspend function for Venus 3xx versions by >> add a check for WFI (wait for interrupt) bit. This bit >> is on when the ARM9 is idle and entered in low power mode. > >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/hfi_venus.c | 59 > ++++++++++++++++-------- >> drivers/media/platform/qcom/venus/hfi_venus_io.h | 1 + >> 2 files changed, 41 insertions(+), 19 deletions(-) > >> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c > b/drivers/media/platform/qcom/venus/hfi_venus.c >> index 53546174aab8..aac351f699a0 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_venus.c >> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c >> @@ -1447,7 +1447,7 @@ static int venus_suspend_3xx(struct venus_core > *core) >> { >> struct venus_hfi_device *hdev = to_hfi_priv(core); >> struct device *dev = core->dev; >> - u32 ctrl_status, wfi_status; >> + u32 ctrl_status, cpu_status; >> int ret; >> int cnt = 100; > >> @@ -1463,29 +1463,50 @@ static int venus_suspend_3xx(struct venus_core > *core) >> return -EINVAL; >> } > >> - ctrl_status = venus_readl(hdev, CPU_CS_SCIACMDARG0); >> - if (!(ctrl_status & CPU_CS_SCIACMDARG0_PC_READY)) { >> - wfi_status = venus_readl(hdev, WRAPPER_CPU_STATUS); >> + /* >> + * Power collapse sequence for Venus 3xx and 4xx versions: >> + * 1. Check for ARM9 and video core to be idle by checking WFI bit >> + * (bit 0) in CPU status register and by checking Idle (bit > 30) in >> + * Control status register for video core. >> + * 2. Send a command to prepare for power collapse. >> + * 3. Check for WFI and PC_READY bits. >> + */ >> + >> + while (--cnt) { >> + cpu_status = venus_readl(hdev, WRAPPER_CPU_STATUS); >> ctrl_status = venus_readl(hdev, CPU_CS_SCIACMDARG0); > >> - ret = venus_prepare_power_collapse(hdev, false); >> - if (ret) { >> - dev_err(dev, "prepare for power collapse fail > (%d)\n", >> - ret); >> - return ret; >> - } >> + if (cpu_status & WRAPPER_CPU_STATUS_WFI && >> + ctrl_status & CPU_CS_SCIACMDARG0_INIT_IDLE_MSG_MASK) >> + break; > >> - cnt = 100; >> - while (cnt--) { >> - wfi_status = venus_readl(hdev, > WRAPPER_CPU_STATUS); >> - ctrl_status = venus_readl(hdev, > CPU_CS_SCIACMDARG0); >> - if (ctrl_status & CPU_CS_SCIACMDARG0_PC_READY && >> - wfi_status & BIT(0)) >> - break; >> - usleep_range(1000, 1500); >> - } >> + usleep_range(1000, 1500); >> } > > To avoid opencoding the polling, I'd suggest doing a readx_poll_timeout() > trick: I like the idea, will try to rework that and use readx_poll_timeout. <snip> -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On 05/18/2018 04:53 PM, Tomasz Figa wrote: > On Tue, May 15, 2018 at 5:14 PM Stanimir Varbanov < > stanimir.varbanov@linaro.org> wrote: > >> HFI version 4xx can pass more properties in the sequence change >> event, extend the event structure with them. > >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/hfi.h | 9 ++++++ >> drivers/media/platform/qcom/venus/hfi_msgs.c | 46 > ++++++++++++++++++++++++++++ >> 2 files changed, 55 insertions(+) > >> diff --git a/drivers/media/platform/qcom/venus/hfi.h > b/drivers/media/platform/qcom/venus/hfi.h >> index 5466b7d60dd0..21376d93170f 100644 >> --- a/drivers/media/platform/qcom/venus/hfi.h >> +++ b/drivers/media/platform/qcom/venus/hfi.h >> @@ -74,6 +74,15 @@ struct hfi_event_data { >> u32 tag; >> u32 profile; >> u32 level; > > nit; Could we add a comment saying that it showed in 4xx? Sure, I can add a comment. > > [snip] > >> + case HFI_PROPERTY_CONFIG_VDEC_ENTROPY: >> + data_ptr += sizeof(u32); >> + entropy_mode = *(u32 *)data_ptr; >> + event.entropy_mode = entropy_mode; > > Is the |entropy_mode| local variable necessary? Isn't GCC smart enough ;) Sure, I can drop entropy_mode local variable. -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On 05/18/2018 05:16 PM, Tomasz Figa wrote: > On Tue, May 15, 2018 at 5:13 PM Stanimir Varbanov < > stanimir.varbanov@linaro.org> wrote: > >> Adds set_properties method to handle newer 4xx properties and >> fall-back to 3xx for the rest. > >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/hfi_cmds.c | 64 > +++++++++++++++++++++++++++- >> 1 file changed, 63 insertions(+), 1 deletion(-) > >> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c > b/drivers/media/platform/qcom/venus/hfi_cmds.c >> index 1cfeb7743041..6bd287154796 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c >> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c > > [snip] > >> + case HFI_PROPERTY_CONFIG_VENC_MAX_BITRATE: >> + /* not implemented on Venus 4xx */ > > Shouldn't return -EINVAL here, similar to what > pkt_session_set_property_1x() does for unknown property? Probably the right error code should be ENOTSUPP, but I kind of following the rule to silently not return the error to simplify the callers of set_property (otherwise I have to have a version conditional code in the callers). > >> + break; >> + default: >> + ret = pkt_session_set_property_3xx(pkt, cookie, ptype, > pdata); >> + break; > > nit: How about simply return pkt_session_set_property_3xx(pkt, cookie, > ptype, pdata); and removing the |ret| variable completely, since the return > below the switch can just return 0 all the time? OK, I will do that way. > >> + } >> + >> + return ret; >> +} >> + >> int pkt_session_get_property(struct hfi_session_get_property_pkt *pkt, >> void *cookie, u32 ptype) >> { >> @@ -1181,7 +1240,10 @@ int pkt_session_set_property(struct > hfi_session_set_property_pkt *pkt, >> if (hfi_ver == HFI_VERSION_1XX) >> return pkt_session_set_property_1x(pkt, cookie, ptype, > pdata); > >> - return pkt_session_set_property_3xx(pkt, cookie, ptype, pdata); >> + if (hfi_ver == HFI_VERSION_3XX) >> + return pkt_session_set_property_3xx(pkt, cookie, ptype, > pdata); >> + >> + return pkt_session_set_property_4xx(pkt, cookie, ptype, pdata); > > nit: Since we're adding third variant, I'd consider using function pointers > here, but no strong opinion. Let's keep that for future improvements. -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On 05/24/2018 09:11 AM, Tomasz Figa wrote: > Hi Stanimir, > > On Tue, May 15, 2018 at 5:10 PM Stanimir Varbanov < > stanimir.varbanov@linaro.org> wrote: > >> This extends the clocks number to support suspend and resume >> on Venus version 4xx. > >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 4 +-- >> drivers/media/platform/qcom/venus/vdec.c | 42 > ++++++++++++++++++++++++++------ >> drivers/media/platform/qcom/venus/venc.c | 42 > ++++++++++++++++++++++++++------ >> 3 files changed, 72 insertions(+), 16 deletions(-) > >> diff --git a/drivers/media/platform/qcom/venus/core.h > b/drivers/media/platform/qcom/venus/core.h >> index 8d3e150800c9..b5b9a84e9155 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -92,8 +92,8 @@ struct venus_core { >> void __iomem *base; >> int irq; >> struct clk *clks[VIDC_CLKS_NUM_MAX]; >> - struct clk *core0_clk; >> - struct clk *core1_clk; >> + struct clk *core0_clk, *core0_bus_clk; >> + struct clk *core1_clk, *core1_bus_clk; >> struct video_device *vdev_dec; >> struct video_device *vdev_enc; >> struct v4l2_device v4l2_dev; >> diff --git a/drivers/media/platform/qcom/venus/vdec.c > b/drivers/media/platform/qcom/venus/vdec.c >> index 261a51adeef2..c45452634e7e 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -1081,12 +1081,18 @@ static int vdec_probe(struct platform_device > *pdev) >> if (!core) >> return -EPROBE_DEFER; > >> - if (core->res->hfi_version == HFI_VERSION_3XX) { >> + if (IS_V3(core) || IS_V4(core)) { >> core->core0_clk = devm_clk_get(dev, "core"); >> if (IS_ERR(core->core0_clk)) >> return PTR_ERR(core->core0_clk); >> } > >> + if (IS_V4(core)) { >> + core->core0_bus_clk = devm_clk_get(dev, "bus"); >> + if (IS_ERR(core->core0_bus_clk)) >> + return PTR_ERR(core->core0_bus_clk); >> + } >> + > > Rather than doing this conditional dance, wouldn't it make more sense to > just list all the clocks in variant data struct and use clk_bulk_get()? Do you mean the same as it is done for venus/core.c ? > >> platform_set_drvdata(pdev, core); > >> vdev = video_device_alloc(); >> @@ -1132,12 +1138,23 @@ static __maybe_unused int > vdec_runtime_suspend(struct device *dev) >> { >> struct venus_core *core = dev_get_drvdata(dev); > >> - if (core->res->hfi_version == HFI_VERSION_1XX) >> + if (IS_V1(core)) >> return 0; > >> - writel(0, core->base + WRAPPER_VDEC_VCODEC_POWER_CONTROL); >> + if (IS_V3(core)) >> + writel(0, core->base + WRAPPER_VDEC_VCODEC_POWER_CONTROL); >> + else if (IS_V4(core)) >> + writel(0, core->base + > WRAPPER_VCODEC0_MMCC_POWER_CONTROL); >> + >> + if (IS_V4(core)) >> + clk_disable_unprepare(core->core0_bus_clk); >> + >> clk_disable_unprepare(core->core0_clk); >> - writel(1, core->base + WRAPPER_VDEC_VCODEC_POWER_CONTROL); >> + >> + if (IS_V3(core)) >> + writel(1, core->base + WRAPPER_VDEC_VCODEC_POWER_CONTROL); >> + else if (IS_V4(core)) >> + writel(1, core->base + > WRAPPER_VCODEC0_MMCC_POWER_CONTROL); > > Almost every step here differs between version. I'd suggest splitting this > into separate functions for both versions. I think it will be better to squash this patch with 13/29. -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 < > 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. OK, will rework that part. > > [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 > > [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. will add a description. > >> 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.) I just followed the other code examples in the kernel and in venus. If you insist I can drop 'inline'. > >> +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? yes, will correct. > >> + 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? OK. And also will drop num_properties because it is not used by hfi_parser(). > > [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? It comes from firmware through HFI interface. And yes we can check for such anomalies. > >> + error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES; >> + goto done; >> + } > >> + error = hfi_parser(core, inst, pkt->num_properties, &pkt->data[0], >> + rem_bytes); > > nit: pkt->data? OK. > >> 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.) yes, it comes from firmware message 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()? OK, I'll give it a try. > >> + 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. Correct. Will fix it. > > Also BIT(i). Sure will use BIT(). > >> + 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. yes, we need to check the domain because we can have the same codec for both domains decoder and encoder but with different capabilities. > > 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. sure, I forgot to add define for that. > >> + 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? yes > >> + count--; >> + } > > Am I missing something or this function doesn’t to do anything? yes, currently it is not used. I'll update it. > >> +} >> + >> +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' > >> + 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)); > > ? yes it is. > >> + >> + 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? 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. > >> + >> + 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. > >> + >> + 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. > >> + 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. > >> + } >> + >> + 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? most probably, will fix it. > >> + if (caps->caps[i].capability_type == type) >> + return &caps->caps[i]; >> + } >> + >> + return ERR_PTR(-EINVAL); >> +} -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 31, 2018 at 1:21 AM Stanimir Varbanov <stanimir.varbanov@linaro.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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, Thanks for the review! On 05/31/2018 10:59 AM, Tomasz Figa wrote: > On Tue, May 15, 2018 at 5:06 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> Rename is_reg_unreg_needed() to better name is_dynamic_bufmode() and >> use buffer mode from enumerated per codec capabilities. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/helpers.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c >> index 2b21f6ed7502..1eda19adbf28 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.c >> +++ b/drivers/media/platform/qcom/venus/helpers.c >> @@ -354,18 +354,19 @@ session_process_buf(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf) >> return 0; >> } >> >> -static inline int is_reg_unreg_needed(struct venus_inst *inst) >> +static inline int is_dynamic_bufmode(struct venus_inst *inst) > > nit: Could be made bool. And drop inline I guess? :) -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 15, 2018 at 5:01 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Make a new function vdec_output_conf() for decoder output > configuration. vdec_output_conf() will set properties via > HFI interface related to the output configuration, and > keep vdec_set_properties() which will set properties > related to decoding parameters. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/vdec.c | 35 ++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index 5a5e3e2fece4..3a699af0ab58 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -545,6 +545,23 @@ static const struct v4l2_ioctl_ops vdec_ioctl_ops = { > static int vdec_set_properties(struct venus_inst *inst) > { > struct vdec_controls *ctr = &inst->controls.dec; > + struct hfi_enable en = { .enable = 1 }; > + u32 ptype; > + int ret; > + > + if (ctr->post_loop_deb_mode) { > + ptype = HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER; > + en.enable = 1; en.enable was already set to 1 in the definition. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stanimir, On Tue, May 15, 2018 at 5:14 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hello, > > Here is v2 with following comments addressed: > > * reworked venus suspend 3xx and reuse it for 4xx. > * drop 10/28 patch from v1, i.e. call of session_continue when > buffer requirements are not sufficient. > * fixed kbuild test robot warning in 11/28 by allocating instance > variable from heap. > * spelling typo in 15/28. > * added Reviewed-by for DT changes. > * extended 28/28 HEVC support for encoder, now the profile and > level are selected properly. > > Comments are welcome! Thanks a lot for the series. I finally managed to finish reviewing it. Sorry for taking so long. For the patches I didn't send any comments for, please feel free to add my Reviewed-by. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stanimir, On Thu, May 31, 2018 at 6:51 PM Tomasz Figa <tfiga@chromium.org> wrote: > > On Tue, May 15, 2018 at 5:00 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: > > > > This is implementing a multi-stream decoder support. The multi > > stream gives an option to use the secondary decoder output > > with different raw format (or the same in case of crop). > > > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > > --- > > drivers/media/platform/qcom/venus/core.h | 1 + > > drivers/media/platform/qcom/venus/helpers.c | 204 +++++++++++++++++++++++++++- > > drivers/media/platform/qcom/venus/helpers.h | 6 + > > drivers/media/platform/qcom/venus/vdec.c | 91 ++++++++++++- > > drivers/media/platform/qcom/venus/venc.c | 1 + > > 5 files changed, 299 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > > index 4d6c05f156c4..85e66e2dd672 100644 > > --- a/drivers/media/platform/qcom/venus/core.h > > +++ b/drivers/media/platform/qcom/venus/core.h > > @@ -259,6 +259,7 @@ struct venus_inst { > > struct list_head list; > > struct mutex lock; > > struct venus_core *core; > > + struct list_head dpbbufs; > > struct list_head internalbufs; > > struct list_head registeredbufs; > > struct list_head delayed_process; > > diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c > > index ed569705ecac..87dcf9973e6f 100644 > > --- a/drivers/media/platform/qcom/venus/helpers.c > > +++ b/drivers/media/platform/qcom/venus/helpers.c > > @@ -85,6 +85,112 @@ bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt) > > } > > EXPORT_SYMBOL_GPL(venus_helper_check_codec); > > > > +static int venus_helper_queue_dpb_bufs(struct venus_inst *inst) > > +{ > > + struct intbuf *buf; > > + int ret = 0; > > + > > + if (list_empty(&inst->dpbbufs)) > > + return 0; > > Does this special case give us anything other than few more source lines? > > > + > > + list_for_each_entry(buf, &inst->dpbbufs, list) { > > + struct hfi_frame_data fdata; > > + > > + memset(&fdata, 0, sizeof(fdata)); > > + fdata.alloc_len = buf->size; > > + fdata.device_addr = buf->da; > > + fdata.buffer_type = buf->type; > > + > > + ret = hfi_session_process_buf(inst, &fdata); > > + if (ret) > > + goto fail; > > + } > > + > > +fail: > > + return ret; > > +} > > + > > +int venus_helper_free_dpb_bufs(struct venus_inst *inst) > > +{ > > + struct intbuf *buf, *n; > > + > > + if (list_empty(&inst->dpbbufs)) > > + return 0; > > Ditto. > > > + > > + list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) { > > + list_del_init(&buf->list); > > + dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da, > > + buf->attrs); > > + kfree(buf); > > + } > > + > > + INIT_LIST_HEAD(&inst->dpbbufs); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(venus_helper_free_dpb_bufs); > [snip] > > +int venus_helper_get_out_fmts(struct venus_inst *inst, u32 v4l2_fmt, > > + u32 *out_fmt, u32 *out2_fmt, bool ubwc) > > +{ > > + struct venus_core *core = inst->core; > > + struct venus_caps *caps; > > + u32 ubwc_fmt, fmt = to_hfi_raw_fmt(v4l2_fmt); > > + bool found, found_ubwc; > > + > > + *out_fmt = *out2_fmt = 0; > > + > > + if (!fmt) > > + return -EINVAL; > > + > > + caps = venus_caps_by_codec(core, inst->hfi_codec, inst->session_type); > > + if (!caps) > > + return -EINVAL; > > + > > + if (ubwc) { > > + ubwc_fmt = fmt | HFI_COLOR_FORMAT_UBWC_BASE; > > Does the UBWC base format have to be the same as fmt? Looking at > HFI_COLOR_FORMAT_* macros, UBWC variants seem to exist only for few > selected raw formats, for example there is none for NV21. Ping. None of the comments I posted above have been addressed in v4. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, On 05/31/2018 12:51 PM, Tomasz Figa wrote: > On Tue, May 15, 2018 at 5:00 PM Stanimir Varbanov > <stanimir.varbanov@linaro.org> wrote: >> >> This is implementing a multi-stream decoder support. The multi >> stream gives an option to use the secondary decoder output >> with different raw format (or the same in case of crop). >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.h | 1 + >> drivers/media/platform/qcom/venus/helpers.c | 204 +++++++++++++++++++++++++++- >> drivers/media/platform/qcom/venus/helpers.h | 6 + >> drivers/media/platform/qcom/venus/vdec.c | 91 ++++++++++++- >> drivers/media/platform/qcom/venus/venc.c | 1 + >> 5 files changed, 299 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h >> index 4d6c05f156c4..85e66e2dd672 100644 >> --- a/drivers/media/platform/qcom/venus/core.h >> +++ b/drivers/media/platform/qcom/venus/core.h >> @@ -259,6 +259,7 @@ struct venus_inst { >> struct list_head list; >> struct mutex lock; >> struct venus_core *core; >> + struct list_head dpbbufs; >> struct list_head internalbufs; >> struct list_head registeredbufs; >> struct list_head delayed_process; >> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c >> index ed569705ecac..87dcf9973e6f 100644 >> --- a/drivers/media/platform/qcom/venus/helpers.c >> +++ b/drivers/media/platform/qcom/venus/helpers.c >> @@ -85,6 +85,112 @@ bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt) >> } >> EXPORT_SYMBOL_GPL(venus_helper_check_codec); >> >> +static int venus_helper_queue_dpb_bufs(struct venus_inst *inst) >> +{ >> + struct intbuf *buf; >> + int ret = 0; >> + >> + if (list_empty(&inst->dpbbufs)) >> + return 0; > > Does this special case give us anything other than few more source lines? yes, thanks for spotting, will drop above lines here and below. > >> + >> + list_for_each_entry(buf, &inst->dpbbufs, list) { >> + struct hfi_frame_data fdata; >> + >> + memset(&fdata, 0, sizeof(fdata)); >> + fdata.alloc_len = buf->size; >> + fdata.device_addr = buf->da; >> + fdata.buffer_type = buf->type; >> + >> + ret = hfi_session_process_buf(inst, &fdata); >> + if (ret) >> + goto fail; >> + } >> + >> +fail: >> + return ret; >> +} >> + >> +int venus_helper_free_dpb_bufs(struct venus_inst *inst) >> +{ >> + struct intbuf *buf, *n; >> + >> + if (list_empty(&inst->dpbbufs)) >> + return 0; > > Ditto. > >> + >> + list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) { >> + list_del_init(&buf->list); >> + dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da, >> + buf->attrs); >> + kfree(buf); >> + } >> + >> + INIT_LIST_HEAD(&inst->dpbbufs); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(venus_helper_free_dpb_bufs); > [snip] >> +int venus_helper_get_out_fmts(struct venus_inst *inst, u32 v4l2_fmt, >> + u32 *out_fmt, u32 *out2_fmt, bool ubwc) >> +{ >> + struct venus_core *core = inst->core; >> + struct venus_caps *caps; >> + u32 ubwc_fmt, fmt = to_hfi_raw_fmt(v4l2_fmt); >> + bool found, found_ubwc; >> + >> + *out_fmt = *out2_fmt = 0; >> + >> + if (!fmt) >> + return -EINVAL; >> + >> + caps = venus_caps_by_codec(core, inst->hfi_codec, inst->session_type); >> + if (!caps) >> + return -EINVAL; >> + >> + if (ubwc) { >> + ubwc_fmt = fmt | HFI_COLOR_FORMAT_UBWC_BASE; > > Does the UBWC base format have to be the same as fmt? Looking at > HFI_COLOR_FORMAT_* macros, UBWC variants seem to exist only for few > selected raw formats, for example there is none for NV21. I think any raw format can have its UBWC variant. And yes we have only one macro but we are checking against parsed capabilities from firmware where the supported formats are stored. -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 5, 2018 at 6:45 PM Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote: > > Hi Tomasz, > > On 07/02/2018 01:05 PM, Tomasz Figa wrote: > > On Mon, Jul 2, 2018 at 6:59 PM Stanimir Varbanov > > <stanimir.varbanov@linaro.org> wrote: > >> > >> Hi Tomasz, > >> > >> On 07/02/2018 12:23 PM, Tomasz Figa wrote: > >>> On Thu, May 31, 2018 at 4:06 PM Tomasz Figa <tfiga@chromium.org> wrote: > >>>> > >>>> On Thu, May 31, 2018 at 1:21 AM Stanimir Varbanov > >>>> <stanimir.varbanov@linaro.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] > >>>>>> > >>>>>>> + 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? > >>> > >>> Ping? > >> > >> OK, you are right there is guarantee that we not mixing keywords and > > > > Did the auto-correction engine in my head got this correctly as "no > > guarantee"? :) > > > >> data. I can make parse_* functions to return how words they consumed and > >> increment 'word' pointer with consumed words. > > > > Yes, that or maybe just returning the pointer to the first word after > > consumed data. Most of the looping functions already seem to have this > > value, so it would have to be just returned. (vs having to subtract > > from the start pointer) > > I made the relevant changes to satisfy you request but the results were > fine for Venus v3 and wrong on v1. So I'd propose to postpone this > change and fix it with follow up patches because I don't want miss the > next merge window. So far the supported venus firmware versions are fine > with the current parser implementation. > > What you think? Fair enough. Generally with the design of those metadata, fixing this problem seems to be quite non-trivial. Let's keep it as is for now. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html