From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Dikshita Agarwal <quic_dikshita@quicinc.com>,
<linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<stanimir.k.varbanov@gmail.com>, <quic_vgarodia@quicinc.com>,
<agross@kernel.org>, <andersson@kernel.org>,
<konrad.dybcio@linaro.org>, <mchehab@kernel.org>,
<bryan.odonoghue@linaro.org>, <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v2 01/34] media: introduce common helpers for video firmware handling
Date: Wed, 20 Dec 2023 13:03:51 -0800 [thread overview]
Message-ID: <db8a9543-2215-7d4c-1cc7-d5ec35dfe540@quicinc.com> (raw)
In-Reply-To: <CAA8EJpqO62HVzZnnu_f3OKsy939N_ckNk_KfR6EuaTxDkhwCjg@mail.gmail.com>
On 12/20/2023 12:56 PM, Dmitry Baryshkov wrote:
> Hi Abhinav,
>
> On Wed, 20 Dec 2023 at 19:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Dmitry
>>
>> On 12/20/2023 12:12 AM, Dmitry Baryshkov wrote:
>>> On Wed, 20 Dec 2023 at 10:01, Dikshita Agarwal
>>> <quic_dikshita@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/18/2023 11:54 PM, Dmitry Baryshkov wrote:
>>>>> On 18/12/2023 13:31, Dikshita Agarwal wrote:
>>>>>> Re-organize the video driver code by introducing a new folder
>>>>>> 'vcodec' and placing 'venus' driver code inside that.
>>>>>>
>>>>>> Introduce common helpers for trustzone based firmware
>>>>>> load/unload etc. which are placed in common folder
>>>>>> i.e. 'vcodec'.
>>>>>> Use these helpers in 'venus' driver. These helpers will be
>>>>>> used by 'iris' driver as well which is introduced later
>>>>>> in this patch series.
>>>>>
>>>>> But why do you need to move the venus driver to subdir?
>>>>
>>>> Currently venus driver is present in drivers/media/platform/qcom folder
>>>> which also has camss folder. We introduced vcodec to keep common code and
>>>> moved venus inside that, to indicate that the common code is for vcodec
>>>> drivers i.e venus and iris. Keeping this in qcom folder would mean, common
>>>> code will be used for camss only which is not the case here.
>>>
>>> you can have .../platform/qcom/camss, .../platform/qcom/vcodec-common,
>>> .../platform/qcom/venus and .../platform/qcom/iris.
>>>
>>> If you were to use build helpers in a proper kernel module, this would
>>> be more obvious.
>>>
>>
>> Although your suggestion is good in terms of avoiding moving venus, I
>> think the location of venus was wrong to begin with. There should have
>> always been a vcodec (or similar) folder for venus/iris as that will
>> establish the boundaries of camss and video sub-system in a cleaner way
>>
>> I like the mediatek separation that way as it makes the boundaries clear:
>>
>> drivers/media/platform/mediatek$ ls
>> jpeg Kconfig Makefile mdp mdp3 vcodec vpu
>>
>> So I think that this re-org of venus into a vcodec had to happen at some
>> point. Its just that it aligned with iris addition.
>
> Then it should be a clean separate step with its own justification.
>
Yes, I am fine with this first going as a separate series to move venus
into vcodec and anyway there was a separate request to make patches 1-3
into another series from Bryan.
So this can first be posted separately.
>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>>>> ---
>>>>>> drivers/media/platform/qcom/Kconfig | 2 +-
>>>>>> drivers/media/platform/qcom/Makefile | 2 +-
>>>>>> drivers/media/platform/qcom/vcodec/firmware.c | 147 +++++++++
>>>>>> drivers/media/platform/qcom/vcodec/firmware.h | 21 ++
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/Kconfig | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/Makefile | 4 +-
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/core.c | 102 +++++-
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/core.h | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/dbgfs.c | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/dbgfs.h | 0
>>>>>> .../platform/qcom/vcodec/venus/firmware_no_tz.c | 194 +++++++++++
>>>>>> .../platform/qcom/vcodec/venus/firmware_no_tz.h | 19 ++
>>>>>> .../platform/qcom/{ => vcodec}/venus/helpers.c | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/helpers.h | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/hfi.c | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/hfi.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_cmds.c | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_cmds.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_helper.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_msgs.c | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_msgs.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_parser.c | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_parser.h | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_plat_bufs.h | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_plat_bufs_v6.c | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_platform.c | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_platform.h | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_platform_v4.c | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_platform_v6.c | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_venus.c | 21 +-
>>>>>> .../platform/qcom/{ => vcodec}/venus/hfi_venus.h | 0
>>>>>> .../qcom/{ => vcodec}/venus/hfi_venus_io.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/pm_helpers.c | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/pm_helpers.h | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/vdec.c | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/vdec.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/vdec_ctrls.c | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/venc.c | 0
>>>>>> .../media/platform/qcom/{ => vcodec}/venus/venc.h | 0
>>>>>> .../platform/qcom/{ => vcodec}/venus/venc_ctrls.c | 0
>>>>>> drivers/media/platform/qcom/venus/firmware.c | 363
>>>>>> ---------------------
>>>>>> drivers/media/platform/qcom/venus/firmware.h | 26 --
>>>>>> 42 files changed, 492 insertions(+), 409 deletions(-)
>>>>>> create mode 100644 drivers/media/platform/qcom/vcodec/firmware.c
>>>>>> create mode 100644 drivers/media/platform/qcom/vcodec/firmware.h
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/Kconfig (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/Makefile (83%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/core.c (91%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/core.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/dbgfs.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/dbgfs.h (100%)
>>>>>> create mode 100644
>>>>>> drivers/media/platform/qcom/vcodec/venus/firmware_no_tz.c
>>>>>> create mode 100644
>>>>>> drivers/media/platform/qcom/vcodec/venus/firmware_no_tz.h
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/helpers.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/helpers.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_cmds.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_cmds.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_helper.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_msgs.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_msgs.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_parser.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_parser.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_plat_bufs.h
>>>>>> (100%)
>>>>>> rename drivers/media/platform/qcom/{ =>
>>>>>> vcodec}/venus/hfi_plat_bufs_v6.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform.c
>>>>>> (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform.h
>>>>>> (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform_v4.c
>>>>>> (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_platform_v6.c
>>>>>> (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_venus.c (99%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_venus.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/hfi_venus_io.h
>>>>>> (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/pm_helpers.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/pm_helpers.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/vdec.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/vdec.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/vdec_ctrls.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/venc.c (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/venc.h (100%)
>>>>>> rename drivers/media/platform/qcom/{ => vcodec}/venus/venc_ctrls.c (100%)
>>>>>> delete mode 100644 drivers/media/platform/qcom/venus/firmware.c
>>>>>> delete mode 100644 drivers/media/platform/qcom/venus/firmware.h
>>>>>>
>>>>>> diff --git a/drivers/media/platform/qcom/Kconfig
>>>>>> b/drivers/media/platform/qcom/Kconfig
>>>>>> index cc5799b..e94142f 100644
>>>>>> --- a/drivers/media/platform/qcom/Kconfig
>>>>>> +++ b/drivers/media/platform/qcom/Kconfig
>>>>>> @@ -3,4 +3,4 @@
>>>>>> comment "Qualcomm media platform drivers"
>>>>>> source "drivers/media/platform/qcom/camss/Kconfig"
>>>>>> -source "drivers/media/platform/qcom/venus/Kconfig"
>>>>>> +source "drivers/media/platform/qcom/vcodec/venus/Kconfig"
>>>>>> diff --git a/drivers/media/platform/qcom/Makefile
>>>>>> b/drivers/media/platform/qcom/Makefile
>>>>>> index 4f055c3..3d2d82b 100644
>>>>>> --- a/drivers/media/platform/qcom/Makefile
>>>>>> +++ b/drivers/media/platform/qcom/Makefile
>>>>>> @@ -1,3 +1,3 @@
>>>>>> # SPDX-License-Identifier: GPL-2.0-only
>>>>>> obj-y += camss/
>>>>>> -obj-y += venus/
>>>>>> +obj-y += vcodec/venus/
>>>>>> diff --git a/drivers/media/platform/qcom/vcodec/firmware.c
>>>>>> b/drivers/media/platform/qcom/vcodec/firmware.c
>>>>>> new file mode 100644
>>>>>> index 0000000..dbc220a
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/media/platform/qcom/vcodec/firmware.c
>>>>>> @@ -0,0 +1,147 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>> +/*
>>>>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights
>>>>>> reserved.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/device.h>
>>>>>> +#include <linux/dma-mapping.h>
>>>>>> +#include <linux/firmware.h>
>>>>>> +#include <linux/kernel.h>
>>>>>> +#include <linux/iommu.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/firmware/qcom/qcom_scm.h>
>>>>>> +#include <linux/of_reserved_mem.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/soc/qcom/mdt_loader.h>
>>>>>> +
>>>>>> +#include "firmware.h"
>>>>>> +
>>>>>> +bool use_tz(struct device *core_dev)
>>>>>
>>>>> All these functions must get some sane prefix. Otherwise a generic 'use_tz'
>>>>> function is too polluting for the global namespace.
>>>>>
>>>> I understand, will check and do the needful.
>>>>>> +{
>>>>>> + struct device_node *np;
>>>>>> +
>>>>>> + np = of_get_child_by_name(core_dev->of_node, "video-firmware");
>>>>>> + if (!np)
>>>>>> + return true;
>>>>>> +
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> +int protect_secure_region(u32 cp_start, u32 cp_size, u32 cp_nonpixel_start,
>>>>>> + u32 cp_nonpixel_size, u32 pas_id)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + /*
>>>>>> + * Clues for porting using downstream data:
>>>>>> + * cp_start = 0
>>>>>> + * cp_size = venus_ns/virtual-addr-pool[0] - yes, address and not size!
>>>>>> + * This works, as the non-secure context bank is placed
>>>>>> + * contiguously right after the Content Protection region.
>>>>>> + *
>>>>>> + * cp_nonpixel_start = venus_sec_non_pixel/virtual-addr-pool[0]
>>>>>> + * cp_nonpixel_size = venus_sec_non_pixel/virtual-addr-pool[1]
>>>>>> + */
>>>>>> + ret = qcom_scm_mem_protect_video_var(cp_start,
>>>>>> + cp_size,
>>>>>> + cp_nonpixel_start,
>>>>>> + cp_nonpixel_size);
>>>>>> + if (ret)
>>>>>> + qcom_scm_pas_shutdown(pas_id);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +int load_fw(struct device *dev, const char *fw_name, phys_addr_t *mem_phys,
>>>>>> + size_t *mem_size, u32 pas_id, bool use_tz)
>>>>>> +{
>>>>>> + const struct firmware *firmware = NULL;
>>>>>> + struct reserved_mem *rmem;
>>>>>> + struct device_node *node;
>>>>>> + void *mem_virt = NULL;
>>>>>> + ssize_t fw_size = 0;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
>>>>>
>>>>> Why? Can you just depend on it?
>>>>>
>>>> Sure, Will check this and get back.
>>>>>> + (use_tz && !qcom_scm_is_available()))
>>>>>> + return -EPROBE_DEFER;
>>>>>> +
>>>>>> + if (!fw_name || !(*fw_name))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + *mem_phys = 0;
>>>>>> + *mem_size = 0;
>>>>>> +
>>>>>> + node = of_parse_phandle(dev->of_node, "memory-region", 0);
>>>>>> + if (!node) {
>>>>>> + dev_err(dev, "no memory-region specified\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + rmem = of_reserved_mem_lookup(node);
>>>>>> + of_node_put(node);
>>>>>> + if (!rmem) {
>>>>>> + dev_err(dev, "failed to lookup reserved memory-region\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + ret = request_firmware(&firmware, fw_name, dev);
>>>>>> + if (ret) {
>>>>>> + dev_err(dev, "%s: failed to request fw \"%s\", error %d\n",
>>>>>> + __func__, fw_name, ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + fw_size = qcom_mdt_get_size(firmware);
>>>>>> + if (fw_size < 0) {
>>>>>> + ret = fw_size;
>>>>>> + dev_err(dev, "%s: out of bound fw image fw size: %ld\n",
>>>>>> + __func__, fw_size);
>>>>>> + goto err_release_fw;
>>>>>> + }
>>>>>> +
>>>>>> + *mem_phys = rmem->base;
>>>>>> + *mem_size = rmem->size;
>>>>>> +
>>>>>> + if (*mem_size < fw_size) {
>>>>>> + ret = -EINVAL;
>>>>>> + goto err_release_fw;
>>>>>> + }
>>>>>> +
>>>>>> + mem_virt = memremap(*mem_phys, *mem_size, MEMREMAP_WC);
>>>>>> + if (!mem_virt) {
>>>>>> + dev_err(dev, "unable to remap fw memory region %pa size %#zx\n",
>>>>>> + mem_phys, *mem_size);
>>>>>> + goto err_release_fw;
>>>>>> + }
>>>>>> +
>>>>>> + if (use_tz)
>>>>>> + ret = qcom_mdt_load(dev, firmware, fw_name, pas_id, mem_virt,
>>>>>> + *mem_phys, *mem_size, NULL);
>>>>>> + else
>>>>>> + ret = qcom_mdt_load_no_init(dev, firmware, fw_name, pas_id,
>>>>>> mem_virt,
>>>>>> + *mem_phys, *mem_size, NULL);
>>>>>> + if (ret) {
>>>>>> + dev_err(dev, "%s: error %d loading fw \"%s\"\n",
>>>>>> + __func__, ret, fw_name);
>>>>>> + }
>>>>>> +
>>>>>> + memunmap(mem_virt);
>>>>>> +err_release_fw:
>>>>>> + release_firmware(firmware);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +int auth_reset_fw(u32 pas_id)
>>>>>> +{
>>>>>> + return qcom_scm_pas_auth_and_reset(pas_id);
>>>>>> +}
>>>>>> +
>>>>>> +void unload_fw(u32 pas_id)
>>>>>> +{
>>>>>> + qcom_scm_pas_shutdown(pas_id);
>>>>>> +}
>>>>>> +
>>>>>> +int set_hw_state(bool resume)
>>>>>> +{
>>>>>> + return qcom_scm_set_remote_state(resume, 0);
>>>>>> +}
>>>>>> diff --git a/drivers/media/platform/qcom/vcodec/firmware.h
>>>>>> b/drivers/media/platform/qcom/vcodec/firmware.h
>>>>>> new file mode 100644
>>>>>> index 0000000..7d410a8
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/media/platform/qcom/vcodec/firmware.h
>>>>>> @@ -0,0 +1,21 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>>> +/*
>>>>>> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights
>>>>>> reserved.
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef _FIRMWARE_H_
>>>>>> +#define _FIRMWARE_H_
>>>>>> +
>>>>>> +#include <linux/device.h>
>>>>>> +#include <linux/types.h>
>>>>>> +
>>>>>> +bool use_tz(struct device *core_dev);
>>>>>> +int load_fw(struct device *dev, const char *fw_name, phys_addr_t *mem_phys,
>>>>>> + size_t *mem_size, u32 pas_id, bool use_tz);
>>>>>> +int auth_reset_fw(u32 pas_id);
>>>>>> +int protect_secure_region(u32 cp_start, u32 cp_size, u32 cp_nonpixel_start,
>>>>>> + u32 cp_nonpixel_size, u32 pas_id);
>>>>>> +void unload_fw(u32 pas_id);
>>>>>> +int set_hw_state(bool resume);
>>>>>> +
>>>>>> +#endif
>>>>>> diff --git a/drivers/media/platform/qcom/venus/Kconfig
>>>>>> b/drivers/media/platform/qcom/vcodec/venus/Kconfig
>>>>>> similarity index 100%
>>>>>> rename from drivers/media/platform/qcom/venus/Kconfig
>>>>>> rename to drivers/media/platform/qcom/vcodec/venus/Kconfig
>>>>>> diff --git a/drivers/media/platform/qcom/venus/Makefile
>>>>>> b/drivers/media/platform/qcom/vcodec/venus/Makefile
>>>>>> similarity index 83%
>>>>>> rename from drivers/media/platform/qcom/venus/Makefile
>>>>>> rename to drivers/media/platform/qcom/vcodec/venus/Makefile
>>>>>> index 91ee6be..f6f3a88 100644
>>>>>> --- a/drivers/media/platform/qcom/venus/Makefile
>>>>>> +++ b/drivers/media/platform/qcom/vcodec/venus/Makefile
>>>>>> @@ -1,7 +1,9 @@
>>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>>> # Makefile for Qualcomm Venus driver
>>>>>> -venus-core-objs += core.o helpers.o firmware.o \
>>>>>> +venus-core-objs += ../firmware.o
>>>>>> +
>>>>>> +venus-core-objs += core.o helpers.o firmware_no_tz.o \
>>>>>> hfi_venus.o hfi_msgs.o hfi_cmds.o hfi.o \
>>>>>> hfi_parser.o pm_helpers.o dbgfs.o \
>>>>>> hfi_platform.o hfi_platform_v4.o \
>>>>>> diff --git a/drivers/media/platform/qcom/venus/core.c
>>>>>> b/drivers/media/platform/qcom/vcodec/venus/core.c
>>>>>> similarity index 91%
>>>>>> rename from drivers/media/platform/qcom/venus/core.c
>>>>>> rename to drivers/media/platform/qcom/vcodec/venus/core.c
>>>>>> index 9cffe97..56d9a53 100644
>>>>>> --- a/drivers/media/platform/qcom/venus/core.c
>>>>>> +++ b/drivers/media/platform/qcom/vcodec/venus/core.c
>>>>>> @@ -22,7 +22,8 @@
>>>>>> #include <media/v4l2-ioctl.h>
>>>>>> #include "core.h"
>>>>>> -#include "firmware.h"
>>>>>> +#include "../firmware.h"
>>>>>> +#include "firmware_no_tz.h"
>>>>>> #include "pm_helpers.h"
>>>>>> #include "hfi_venus_io.h"
>>>>>> @@ -86,6 +87,8 @@ static void venus_sys_error_handler(struct
>>>>>> work_struct *work)
>>>>>> struct venus_core *core =
>>>>>> container_of(work, struct venus_core, work.work);
>>>>>> int ret, i, max_attempts = RPM_WAIT_FOR_IDLE_MAX_ATTEMPTS;
>>>>>> + const struct venus_resources *res = core->res;
>>>>>> + const char *fwpath = NULL;
>>>>>> const char *err_msg = "";
>>>>>> bool failed = false;
>>>>>> @@ -107,7 +110,10 @@ static void venus_sys_error_handler(struct
>>>>>> work_struct *work)
>>>>>> mutex_lock(&core->lock);
>>>>>> - venus_shutdown(core);
>>>>>> + if (core->use_tz)
>>>>>> + unload_fw(VENUS_PAS_ID);
>>>>>> + else
>>>>>> + unload_fw_no_tz(core);
>>>>>
>>>>> This is more than introducing helpers.
>>>>>
>>>> The new helpers are written to make the code generic for video drivers.
>>>> which requires changes in the calling function also.
>>>>>> venus_coredump(core);
>>>>>> @@ -127,12 +133,39 @@ static void venus_sys_error_handler(struct
>>>>>> work_struct *work)
>>>>>> failed = true;
>>>>>> }
>>>>>> - ret = venus_boot(core);
>>>>>> + ret = of_property_read_string_index(core->dev->of_node,
>>>>>> "firmware-name", 0,
>>>>>> + &fwpath);
>>>>>> + if (ret)
>>>>>> + fwpath = core->res->fwname;
>>>>>> +
>>>>>> + ret = load_fw(core->dev, fwpath, &core->fw.mem_phys,
>>>>>> &core->fw.mem_size,
>>>>>> + VENUS_PAS_ID, core->use_tz);
>>>>>
>>>>> So, we had a nice local 'venus_boot'. Instead we now have a pile of code
>>>>> with non-generic prefixes, etc. If you are introducing helpers, please
>>>>> refrain from inlining of calling functions, etc. Just move the code to your
>>>>> helpers.
>>>>>
>>>> As mentioned in above comment, the common helpers are written to make the
>>>> code generic. I Will try to make it more clear, working on the same.
>>>
>>> First, you move the code, then you make it generic. Or vice versa.
>>> First you split the code, then you move it. Don't do both in the same
>>> patch.
>>>
>>>>> NAK for the rest of the patch.
>>>
>>>
>
>
>
next prev parent reply other threads:[~2023-12-20 21:03 UTC|newest]
Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-18 11:31 [PATCH v2 00/34] Qualcomm video encoder and decoder driver Dikshita Agarwal
2023-12-18 11:31 ` [PATCH v2 01/34] media: introduce common helpers for video firmware handling Dikshita Agarwal
2023-12-18 18:24 ` Dmitry Baryshkov
2023-12-20 8:01 ` Dikshita Agarwal
2023-12-20 8:12 ` Dmitry Baryshkov
2023-12-20 17:10 ` Abhinav Kumar
2023-12-20 20:56 ` Dmitry Baryshkov
2023-12-20 21:03 ` Abhinav Kumar [this message]
2023-12-19 11:40 ` Bryan O'Donoghue
2023-12-19 13:26 ` Dmitry Baryshkov
2023-12-20 8:01 ` Dikshita Agarwal
2023-12-18 11:31 ` [PATCH v2 02/34] media: introduce common helpers for queues handling Dikshita Agarwal
2023-12-18 18:29 ` Dmitry Baryshkov
2023-12-20 8:03 ` Dikshita Agarwal
2023-12-18 11:31 ` [PATCH v2 03/34] media: introduce common helpers for buffer size calculation Dikshita Agarwal
2023-12-18 18:32 ` Dmitry Baryshkov
2023-12-20 8:04 ` Dikshita Agarwal
2023-12-20 8:15 ` Dmitry Baryshkov
2023-12-18 11:31 ` [PATCH v2 04/34] dt-bindings: media: Add sm8550 dt schema Dikshita Agarwal
2023-12-18 16:10 ` Krzysztof Kozlowski
2023-12-18 18:17 ` Dmitry Baryshkov
2023-12-18 11:32 ` [PATCH v2 05/34] media: MAINTAINERS: Add Qualcomm Iris video accelerator driver Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 06/34] media: iris: register video device to platform driver Dikshita Agarwal
2023-12-18 18:40 ` Dmitry Baryshkov
2023-12-18 11:32 ` [PATCH v2 07/34] media: iris: initialize power resources Dikshita Agarwal
2023-12-18 15:09 ` Konrad Dybcio
2023-12-20 8:04 ` Dikshita Agarwal
2024-01-03 13:45 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 08/34] media: iris: introduce state machine for iris core Dikshita Agarwal
2023-12-18 18:46 ` Dmitry Baryshkov
2023-12-20 8:05 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 09/34] media: iris: initialize shared queues for host and firmware communication Dikshita Agarwal
2023-12-18 18:46 ` Dmitry Baryshkov
2023-12-20 8:05 ` Dikshita Agarwal
2023-12-18 21:33 ` Konrad Dybcio
2023-12-20 8:05 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 10/34] media: iris: add PIL functionality for video firmware Dikshita Agarwal
2023-12-18 21:40 ` Konrad Dybcio
2023-12-20 8:15 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 11/34] media: iris: introduce packetization layer for creating HFI packets Dikshita Agarwal
2023-12-18 21:50 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 12/34] media: iris: add video processing unit(VPU) specific register handling Dikshita Agarwal
2023-12-18 16:19 ` Krzysztof Kozlowski
2023-12-18 22:00 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 13/34] media: iris: introduce platform specific capabilities for core and instance Dikshita Agarwal
2023-12-18 22:08 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 14/34] media: iris: implement iris v4l2 file ops Dikshita Agarwal
2023-12-19 11:57 ` Bryan O'Donoghue
2023-12-18 11:32 ` [PATCH v2 15/34] media: iris: add handling for interrupt service routine(ISR) invoked by hardware Dikshita Agarwal
2023-12-19 11:48 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 16/34] media: iris: implement iris v4l2_ctrl_ops and prepare capabilities Dikshita Agarwal
2023-12-19 11:50 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 17/34] media: iris: implement vb2_ops queue setup Dikshita Agarwal
2023-12-19 11:56 ` Konrad Dybcio
2023-12-20 8:37 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 18/34] media: iris: introduce and implement iris vb2 mem ops Dikshita Agarwal
2023-12-19 11:58 ` Konrad Dybcio
2023-12-18 11:32 ` [PATCH v2 19/34] media: iris: implement HFI to queue and release buffers Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 20/34] media: iris: add video hardware internal buffer count and size calculation Dikshita Agarwal
2023-12-19 12:06 ` Bryan O'Donoghue
2023-12-20 8:29 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 21/34] media: iris: implement internal buffer management Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 22/34] media: iris: introduce instance states Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 23/34] media: iris: implement iris v4l2 ioctl ops supported by decoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 24/34] media: iris: subscribe input and output properties to firmware Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 25/34] media: iris: subscribe src change and handle firmware responses Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 26/34] media: iris: implement vb2 streaming ops on capture and output planes Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 27/34] media: iris: implement vb2 ops for buf_queue and firmware response Dikshita Agarwal
2023-12-19 12:21 ` Konrad Dybcio
2023-12-20 8:25 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 28/34] media: iris: add instance sub states and implement DRC and Drain sequence Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 29/34] media: iris: implement power management Dikshita Agarwal
2023-12-19 12:24 ` Konrad Dybcio
2023-12-20 8:23 ` Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 30/34] media: iris: register video encoder device to platform driver Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 31/34] media: iris: add platform specific instance capabilities for encoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 32/34] media: iris: implement iris v4l2 ioctl ops supported by encoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 33/34] media: iris: add vb2 streaming and buffer ops for encoder Dikshita Agarwal
2023-12-18 11:32 ` [PATCH v2 34/34] media: iris: add power management " Dikshita Agarwal
2023-12-19 12:26 ` Konrad Dybcio
2023-12-18 12:09 ` [PATCH v2 00/34] Qualcomm video encoder and decoder driver Dikshita Agarwal
2023-12-18 14:36 ` Bryan O'Donoghue
2023-12-18 18:38 ` Dmitry Baryshkov
2023-12-19 12:10 ` Bryan O'Donoghue
2023-12-20 6:32 ` Vikash Garodia
2023-12-20 7:37 ` Dmitry Baryshkov
2023-12-20 8:14 ` Vikash Garodia
2023-12-20 8:39 ` Dmitry Baryshkov
2023-12-20 8:53 ` Vikash Garodia
2023-12-20 9:52 ` Dmitry Baryshkov
2023-12-20 18:55 ` Abhinav Kumar
2023-12-20 21:24 ` Dmitry Baryshkov
2023-12-20 8:15 ` Krzysztof Kozlowski
2023-12-20 8:32 ` Vikash Garodia
2023-12-20 20:51 ` Nicolas Dufresne
2024-02-29 15:09 ` Vikash Garodia
2024-03-12 10:37 ` Hans Verkuil
2024-03-15 13:51 ` Vikash Garodia
2024-04-12 7:13 ` Hyunjun Ko
2024-04-12 13:52 ` Bryan O'Donoghue
2024-05-16 7:57 ` Hyunjun Ko
2024-05-16 8:06 ` Vikash Garodia
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=db8a9543-2215-7d4c-1cc7-d5ec35dfe540@quicinc.com \
--to=quic_abhinavk@quicinc.com \
--cc=agross@kernel.org \
--cc=andersson@kernel.org \
--cc=bryan.odonoghue@linaro.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=konrad.dybcio@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=quic_dikshita@quicinc.com \
--cc=quic_vgarodia@quicinc.com \
--cc=stanimir.k.varbanov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).