From: Tzung-Bi Shih <tzungbi@google.com>
To: Yunfei Dong <yunfei.dong@mediatek.com>
Cc: Alexandre Courbot <acourbot@chromium.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Tzung-Bi Shih <tzungbi@chromium.org>,
Tiffany Lin <tiffany.lin@mediatek.com>,
Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Tomasz Figa <tfiga@google.com>,
Hsin-Yi Wang <hsinyi@chromium.org>,
Fritz Koenig <frkoenig@chromium.org>,
Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
Benjamin Gaignard <benjamin.gaignard@collabora.com>,
Daniel Vetter <daniel@ffwll.ch>,
dri-devel <dri-devel@lists.freedesktop.org>,
Irui Wang <irui.wang@mediatek.com>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
srv_heupstream@mediatek.com, linux-mediatek@lists.infradead.org,
Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH v10, 06/19] media: mtk-vcodec: Manage multi hardware information
Date: Tue, 16 Nov 2021 17:23:49 +0800 [thread overview]
Message-ID: <YZN4pVLDfAU2O893@google.com> (raw)
In-Reply-To: <20211111041500.17363-7-yunfei.dong@mediatek.com>
On Thu, Nov 11, 2021 at 12:14:47PM +0800, Yunfei Dong wrote:
> Register each hardware(subdev) as platform device used to manage each
> hardware information which includes irq/power/clk. The hardware includes
> LAT0, LAT1 and CORE. And call of_platform_populate in main device.
>
> Using subdev_bitmap to record whether each device is register done. Then check
> whether all subdev are register done before open main device.
I can somehow understand what the patch is trying to do but the commit message needs to be rephrased for people to understand the patch.
> --- a/drivers/media/platform/mtk-vcodec/Makefile
> +++ b/drivers/media/platform/mtk-vcodec/Makefile
> @@ -2,7 +2,8 @@
>
> obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> mtk-vcodec-enc.o \
> - mtk-vcodec-common.o
> + mtk-vcodec-common.o \
> + mtk-vcodec-dec-hw.o
Looks better to align to previous lines.
> +static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx *ctx)
> +{
> + struct mtk_vcodec_dev *vdec_dev = ctx->dev;
ctx isn't used in other places. Looks like the function can accept "struct mtk_vcodec_dev *vdec_dev" as the only argument.
> + for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
> + of_id = &mtk_vdec_hw_match[i];
> + subdev_node = of_find_compatible_node(NULL, NULL,
> + of_id->compatible);
> + if (!subdev_node)
> + continue;
Does it really need to continue if one of the sub-devices is not ready?
It depends on whether mtk_vdec_hw_match is a must list or not. For example, if mtk_vdec_hw_match has 4 entries but the DT only has 2 entries, shall it return an error about the entry count mismatch?
> + if (!of_device_is_available(subdev_node)) {
> + of_node_put(subdev_node);
> + dev_err(&pdev->dev, "Fail to get subdev node\n");
> + continue;
The error message shouldn't be "Fail to get ...". Also the same question: does it really need to continue?
> + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
> + vdec_dev->subdev_node[hw_idx] = subdev_node;
I am wondering if it wouldn't need subdev_node. Isn't vdec_dev->subdev_dev sufficient to clue all the things?
> + if (!test_bit(hw_idx, vdec_dev->subdev_bitmap)) {
> + of_node_put(subdev_node);
> + dev_err(&pdev->dev, "Vdec %d is not ready", hw_idx);
> + return -EAGAIN;
> + }
> + of_node_put(subdev_node);
> + }
> +
> + return 0;
> +}
In addition to the question for subdev_node. The function calls of_node_put() for every paths. I am not sure if the function should call of_node_put() in non-error-handling paths (i.e. I thought it needs someone to hold the reference count).
By reading [v10,11/19] media: mtk-vcodec: Generalize power and clock on/off interfaces[1], the mtk_vcodec_get_hw_dev() calls of_node_put() after it gets the hw_pdev. Looks like the code is meant to borrow the reference count to mtk_vcodec_get_hw_dev().
In short, if the subdev_node is designed to borrow reference count to others, mtk_vcodec_subdev_device_check() shouldn't call of_node_put() in non-error-handling paths.
[1]: https://patchwork.linuxtv.org/project/linux-media/patch/20211111041500.17363-12-yunfei.dong@mediatek.com/
> @@ -249,32 +322,10 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
[...]
> + ret = mtk_vcodec_init_dec_resources(dev);
> if (ret) {
> - dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
> - dev->dec_irq,
> - ret);
> - goto err_res;
> + dev_err(&pdev->dev, "Failed to init pm and registers");
The error message makes less sense about mentioning pm and registers.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
[...]
> +static int mtk_vdec_hw_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mtk_vdec_hw_dev *subdev_dev;
> + struct mtk_vcodec_dev *main_dev;
> + const struct of_device_id *of_id;
> + int hw_idx;
> + int ret;
> +
> + if (!dev->parent)
> + return -EPROBE_DEFER;
IIUC, it shouldn't happen because the deivce is populated from main device. Moreover, would it help to defer the probe if dev->parent is NULL?
> + main_dev = dev_get_drvdata(dev->parent);
> + if (!main_dev)
> + return -EPROBE_DEFER;
Share the same concern with comment above.
> +static struct platform_driver mtk_vdec_driver = {
> + .probe = mtk_vdec_hw_probe,
> + .driver = {
> + .name = "mtk-vdec-comp",
> + .of_match_table = mtk_vdec_hw_match,
> + },
> +};
> +
> +module_platform_driver(mtk_vdec_driver);
I prefer to remove the blank line in between mtk_vdec_driver and module_platform_driver.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
[...]
> @@ -423,6 +436,11 @@ struct mtk_vcodec_enc_pdata {
> * @pm: power management control
> * @dec_capability: used to identify decode capability, ex: 4k
> * @enc_capability: used to identify encode capability
> + *
> + * @subdev_dev: subdev hardware device
> + * @subdev_node: subdev node
> + *
> + * @subdev_bitmap: used to record hardware is ready or not
> */
> struct mtk_vcodec_dev {
> struct v4l2_device v4l2_dev;
> @@ -460,6 +478,11 @@ struct mtk_vcodec_dev {
> struct mtk_vcodec_pm pm;
> unsigned int dec_capability;
> unsigned int enc_capability;
> +
> + void *subdev_dev[MTK_VDEC_HW_MAX];
> + struct device_node *subdev_node[MTK_VDEC_HW_MAX];
The same question: if it already has subdev_dev, does it still need subdev_node?
next prev parent reply other threads:[~2021-11-16 9:24 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-11 4:14 [PATCH v10, 00/19] Support multi hardware decode using of_platform_populate Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 01/19] media: mtk-vcodec: Get numbers of register bases from DT Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 02/19] media: mtk-vcodec: Align vcodec wake up interrupt interface Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 03/19] media: mtk-vcodec: Refactor vcodec pm interface Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 04/19] media: mtk-vcodec: export decoder pm functions Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 05/19] media: mtk-vcodec: Support MT8192 Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 06/19] media: mtk-vcodec: Manage multi hardware information Yunfei Dong
2021-11-16 9:23 ` Tzung-Bi Shih [this message]
2021-11-11 4:14 ` [PATCH v10, 07/19] dt-bindings: media: mtk-vcodec: Separate video encoder and decoder dt-bindings Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 08/19] media: mtk-vcodec: Use pure single core for MT8183 Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 09/19] media: mtk-vcodec: Add irq interface for multi hardware Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 10/19] media: mtk-vcodec: Add msg queue feature for lat and core architecture Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 11/19] media: mtk-vcodec: Generalize power and clock on/off interfaces Yunfei Dong
2021-11-16 9:24 ` Tzung-Bi Shih
2021-11-11 4:14 ` [PATCH v10, 12/19] media: mtk-vcodec: Add new interface to lock different hardware Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 13/19] media: mtk-vcodec: Add work queue for core hardware decode Yunfei Dong
2021-11-11 4:14 ` [PATCH v10,14/19] media: mtk-vcodec: Support 34bits dma address for vdec Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 15/19] dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192 Yunfei Dong
2021-11-14 21:56 ` Ezequiel Garcia
2021-11-16 12:05 ` yunfei.dong
2021-11-11 4:14 ` [PATCH v10, 16/19] media: mtk-vcodec: Add core dec and dec end ipi msg Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 17/19] media: mtk-vcodec: Use codec type to separate different hardware Yunfei Dong
2021-11-11 4:14 ` [PATCH v10, 18/19] media: mtk-vcodec: Remove mtk_vcodec_release_dec_pm Yunfei Dong
2021-11-11 4:15 ` [PATCH v10, 19/19] media: mtk-vcodec: Remove mtk_vcodec_release_enc_pm Yunfei Dong
2021-11-14 22:04 ` [PATCH v10, 00/19] Support multi hardware decode using of_platform_populate Ezequiel Garcia
2021-11-16 11:42 ` yunfei.dong
2021-11-16 12:06 ` Ezequiel Garcia
2021-11-16 12:31 ` yunfei.dong
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=YZN4pVLDfAU2O893@google.com \
--to=tzungbi@google.com \
--cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
--cc=acourbot@chromium.org \
--cc=andrew-ct.chen@mediatek.com \
--cc=benjamin.gaignard@collabora.com \
--cc=dafna.hirschfeld@collabora.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=frkoenig@chromium.org \
--cc=hsinyi@chromium.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=irui.wang@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=robh+dt@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=tfiga@google.com \
--cc=tiffany.lin@mediatek.com \
--cc=tzungbi@chromium.org \
--cc=yunfei.dong@mediatek.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 NNTP newsgroup(s).