| Message ID | 20210901083215.25984-1-yunfei.dong@mediatek.com |
|---|---|
| Headers | show |
| Series | Using component framework to support multi hardware decode | expand |
Hi On 01.09.21 10:32, Yunfei Dong wrote: > There are just one core thread, in order to separeate different > hardware, using codec type to separeate it in scp driver. this code seems to relate to the vpu driver not the scp driver. Is there a corresponding code added to the vpu driver that test the codec_type? > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > .../media/platform/mtk-vcodec/vdec_ipi_msg.h | 12 ++++--- > .../media/platform/mtk-vcodec/vdec_vpu_if.c | 34 ++++++++++++++++--- > .../media/platform/mtk-vcodec/vdec_vpu_if.h | 4 +++ > 3 files changed, 41 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > index 9d8079c4f976..c488f0c40190 100644 > --- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > +++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > @@ -35,6 +35,8 @@ enum vdec_ipi_msgid { > * @msg_id : vdec_ipi_msgid > * @vpu_inst_addr : VPU decoder instance address. Used if ABI version < 2. > * @inst_id : instance ID. Used if the ABI version >= 2. > + * @codec_type : Codec fourcc > + * @reserved : reserved param > */ > struct vdec_ap_ipi_cmd { > uint32_t msg_id; > @@ -42,6 +44,8 @@ struct vdec_ap_ipi_cmd { > uint32_t vpu_inst_addr; > uint32_t inst_id; > }; > + uint32_t codec_type; > + uint32_t reserved; > }; > > /** > @@ -59,12 +63,12 @@ struct vdec_vpu_ipi_ack { > /** > * struct vdec_ap_ipi_init - for AP_IPIMSG_DEC_INIT > * @msg_id : AP_IPIMSG_DEC_INIT > - * @reserved : Reserved field > + * @codec_type : Codec fourcc > * @ap_inst_addr : AP video decoder instance address > */ > struct vdec_ap_ipi_init { > uint32_t msg_id; > - uint32_t reserved; > + uint32_t codec_type; > uint64_t ap_inst_addr; > }; > > @@ -77,7 +81,7 @@ struct vdec_ap_ipi_init { > * H264 decoder [0]:buf_sz [1]:nal_start > * VP8 decoder [0]:width/height > * VP9 decoder [0]:profile, [1][2] width/height > - * @reserved : Reserved field > + * @codec_type : Codec fourcc > */ > struct vdec_ap_ipi_dec_start { > uint32_t msg_id; > @@ -86,7 +90,7 @@ struct vdec_ap_ipi_dec_start { > uint32_t inst_id; > }; > uint32_t data[3]; > - uint32_t reserved; > + uint32_t codec_type; > }; > > /** > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > index bfd8e87dceff..c84fac52fe26 100644 > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > @@ -100,18 +100,29 @@ static void vpu_dec_ipi_handler(void *data, unsigned int len, void *priv) > > static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void *msg, int len) > { > - int err; > + int err, id, msgid; > > - mtk_vcodec_debug(vpu, "id=%X", *(uint32_t *)msg); > + msgid = *(uint32_t *)msg; > + mtk_vcodec_debug(vpu, "id=%X", msgid); > > vpu->failure = 0; > vpu->signaled = 0; > > - err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, vpu->id, msg, > + if (vpu->ctx->dev->vdec_pdata->hw_arch == MTK_VDEC_LAT_SINGLE_CORE) { > + if (msgid == AP_IPIMSG_DEC_CORE || > + msgid == AP_IPIMSG_DEC_CORE_END) > + id = vpu->core_id; > + else > + id = vpu->id; > + } else { > + id = vpu->id; > + } > + > + err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, id, msg, > len, 2000); so > if (err) { > mtk_vcodec_err(vpu, "send fail vpu_id=%d msg_id=%X status=%d", > - vpu->id, *(uint32_t *)msg, err); > + id, msgid, err); > return err; > } > > @@ -131,6 +142,7 @@ static int vcodec_send_ap_ipi(struct vdec_vpu_inst *vpu, unsigned int msg_id) > msg.vpu_inst_addr = vpu->inst_addr; > else > msg.inst_id = vpu->inst_id; > + msg.codec_type = vpu->codec_type; > > err = vcodec_vpu_send_msg(vpu, &msg, sizeof(msg)); > mtk_vcodec_debug(vpu, "- id=%X ret=%d", msg_id, err); > @@ -149,14 +161,25 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu) > > err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id, > vpu->handler, "vdec", NULL); > - if (err != 0) { > + if (err) { could be nice to send a patch with other such fixes, anyway it is better to send unrelated fixes in a separate patch > mtk_vcodec_err(vpu, "vpu_ipi_register fail status=%d", err); > return err; > } > > + if (vpu->ctx->dev->vdec_pdata->hw_arch == MTK_VDEC_LAT_SINGLE_CORE) { > + err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, > + vpu->core_id, vpu->handler, > + "vdec", NULL); > + if (err) { > + mtk_vcodec_err(vpu, "vpu_ipi_register core fail status=%d", err); > + return err; > + } > + } > + > memset(&msg, 0, sizeof(msg)); > msg.msg_id = AP_IPIMSG_DEC_INIT; > msg.ap_inst_addr = (unsigned long)vpu; > + msg.codec_type = vpu->codec_type; > > mtk_vcodec_debug(vpu, "vdec_inst=%p", vpu); > > @@ -187,6 +210,7 @@ int vpu_dec_start(struct vdec_vpu_inst *vpu, uint32_t *data, unsigned int len) > > for (i = 0; i < len; i++) > msg.data[i] = data[i]; > + msg.codec_type = vpu->codec_type; I don't see where is the vpu->codec_type initialzied Thanks, Dafna > > err = vcodec_vpu_send_msg(vpu, (void *)&msg, sizeof(msg)); > mtk_vcodec_debug(vpu, "- ret=%d", err); > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > index ae24b75d1649..802660770a87 100644 > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > @@ -14,6 +14,7 @@ struct mtk_vcodec_ctx; > /** > * struct vdec_vpu_inst - VPU instance for video codec > * @id : ipi msg id for each decoder > + * @core_id : core id used to separate different hardware > * @vsi : driver structure allocated by VPU side and shared to AP side > * for control and info share > * @failure : VPU execution result status, 0: success, others: fail > @@ -26,9 +27,11 @@ struct mtk_vcodec_ctx; > * @dev : platform device of VPU > * @wq : wait queue to wait VPU message ack > * @handler : ipi handler for each decoder > + * @codec_type : used codec type to separate different codecs > */ > struct vdec_vpu_inst { > int id; > + int core_id; > void *vsi; > int32_t failure; > uint32_t inst_addr; > @@ -38,6 +41,7 @@ struct vdec_vpu_inst { > struct mtk_vcodec_ctx *ctx; > wait_queue_head_t wq; > mtk_vcodec_ipi_handler handler; > + unsigned int codec_type; > }; > > /** >
On Wed, 2021-09-01 at 14:17 +0200, Dafna Hirschfeld wrote: Hi Dafna, Thanks for your suggestion. > Hi > > On 01.09.21 10:32, Yunfei Dong wrote: > > There are just one core thread, in order to separeate different > > hardware, using codec type to separeate it in scp driver. > > this code seems to relate to the vpu driver not the scp driver. > Is there a corresponding code added to the vpu driver that test the > codec_type? > Vpu is video processor unit, used to connect with micro processor. In mt8173: vdec_vpu_if.c -> mtk_vpu.c -> micro processor In mt8192/mt8183: vdec_vpu_if.c -> mtk_scp.c ->micro processor This init/dec start/dec_end interfaces are the same for vpu and scp. > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > .../media/platform/mtk-vcodec/vdec_ipi_msg.h | 12 ++++--- > > .../media/platform/mtk-vcodec/vdec_vpu_if.c | 34 > > ++++++++++++++++--- > > .../media/platform/mtk-vcodec/vdec_vpu_if.h | 4 +++ > > 3 files changed, 41 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > index 9d8079c4f976..c488f0c40190 100644 > > --- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > +++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > @@ -35,6 +35,8 @@ enum vdec_ipi_msgid { > > * @msg_id : vdec_ipi_msgid > > * @vpu_inst_addr : VPU decoder instance address. Used if ABI > > version < 2. > > * @inst_id : instance ID. Used if the ABI version >= 2. > > + * @codec_type : Codec fourcc > > + * @reserved : reserved param > > */ > > struct vdec_ap_ipi_cmd { > > uint32_t msg_id; > > @@ -42,6 +44,8 @@ struct vdec_ap_ipi_cmd { > > uint32_t vpu_inst_addr; > > uint32_t inst_id; > > }; > > + uint32_t codec_type; > > + uint32_t reserved; > > }; > > > > /** > > @@ -59,12 +63,12 @@ struct vdec_vpu_ipi_ack { > > /** > > * struct vdec_ap_ipi_init - for AP_IPIMSG_DEC_INIT > > * @msg_id : AP_IPIMSG_DEC_INIT > > - * @reserved : Reserved field > > + * @codec_type : Codec fourcc > > * @ap_inst_addr : AP video decoder instance address > > */ > > struct vdec_ap_ipi_init { > > uint32_t msg_id; > > - uint32_t reserved; > > + uint32_t codec_type; > > uint64_t ap_inst_addr; > > }; > > > > @@ -77,7 +81,7 @@ struct vdec_ap_ipi_init { > > * H264 decoder [0]:buf_sz [1]:nal_start > > * VP8 decoder [0]:width/height > > * VP9 decoder [0]:profile, [1][2] width/height > > - * @reserved : Reserved field > > + * @codec_type : Codec fourcc > > */ > > struct vdec_ap_ipi_dec_start { > > uint32_t msg_id; > > @@ -86,7 +90,7 @@ struct vdec_ap_ipi_dec_start { > > uint32_t inst_id; > > }; > > uint32_t data[3]; > > - uint32_t reserved; > > + uint32_t codec_type; > > }; > > > > /** > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > index bfd8e87dceff..c84fac52fe26 100644 > > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > @@ -100,18 +100,29 @@ static void vpu_dec_ipi_handler(void *data, > > unsigned int len, void *priv) > > > > static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void > > *msg, int len) > > { > > - int err; > > + int err, id, msgid; > > > > - mtk_vcodec_debug(vpu, "id=%X", *(uint32_t *)msg); > > + msgid = *(uint32_t *)msg; > > + mtk_vcodec_debug(vpu, "id=%X", msgid); > > > > vpu->failure = 0; > > vpu->signaled = 0; > > > > - err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, vpu- > > >id, msg, > > + if (vpu->ctx->dev->vdec_pdata->hw_arch == > > MTK_VDEC_LAT_SINGLE_CORE) { > > + if (msgid == AP_IPIMSG_DEC_CORE || > > + msgid == AP_IPIMSG_DEC_CORE_END) > > + id = vpu->core_id; > > + else > > + id = vpu->id; > > + } else { > > + id = vpu->id; > > + } > > + > > + err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, id, > > msg, > > len, 2000); > > so > > if (err) { > > mtk_vcodec_err(vpu, "send fail vpu_id=%d msg_id=%X > > status=%d", > > - vpu->id, *(uint32_t *)msg, err); > > + id, msgid, err); > > return err; > > } > > > > @@ -131,6 +142,7 @@ static int vcodec_send_ap_ipi(struct > > vdec_vpu_inst *vpu, unsigned int msg_id) > > msg.vpu_inst_addr = vpu->inst_addr; > > else > > msg.inst_id = vpu->inst_id; > > + msg.codec_type = vpu->codec_type; > > > > err = vcodec_vpu_send_msg(vpu, &msg, sizeof(msg)); > > mtk_vcodec_debug(vpu, "- id=%X ret=%d", msg_id, err); > > @@ -149,14 +161,25 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu) > > > > err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, > > vpu->id, > > vpu->handler, "vdec", NULL); > > - if (err != 0) { > > + if (err) { > > could be nice to send a patch with other such fixes, > anyway it is better to send unrelated fixes in a separate patch > will fix in next patch. > > mtk_vcodec_err(vpu, "vpu_ipi_register fail status=%d", > > err); > > return err; > > } > > > > + if (vpu->ctx->dev->vdec_pdata->hw_arch == > > MTK_VDEC_LAT_SINGLE_CORE) { > > + err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev- > > >fw_handler, > > + vpu->core_id, vpu->handler, > > + "vdec", NULL); > > + if (err) { > > + mtk_vcodec_err(vpu, "vpu_ipi_register core fail > > status=%d", err); > > + return err; > > + } > > + } > > + > > memset(&msg, 0, sizeof(msg)); > > msg.msg_id = AP_IPIMSG_DEC_INIT; > > msg.ap_inst_addr = (unsigned long)vpu; > > + msg.codec_type = vpu->codec_type; > > > > mtk_vcodec_debug(vpu, "vdec_inst=%p", vpu); > > > > @@ -187,6 +210,7 @@ int vpu_dec_start(struct vdec_vpu_inst *vpu, > > uint32_t *data, unsigned int len) > > > > for (i = 0; i < len; i++) > > msg.data[i] = data[i]; > > + msg.codec_type = vpu->codec_type; > > I don't see where is the vpu->codec_type initialzied > This patch just add interface to support core hardware decode, in next serial patches based on these will used codec type to separate after these base patches are stable. > Thanks, > Dafna > Thanks Yunfei Dong > > > > err = vcodec_vpu_send_msg(vpu, (void *)&msg, sizeof(msg)); > > mtk_vcodec_debug(vpu, "- ret=%d", err); > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > index ae24b75d1649..802660770a87 100644 > > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > @@ -14,6 +14,7 @@ struct mtk_vcodec_ctx; > > /** > > * struct vdec_vpu_inst - VPU instance for video codec > > * @id : ipi msg id for each decoder > > + * @core_id : core id used to separate different hardware > > * @vsi : driver structure allocated by VPU side and > > shared to AP side > > * for control and info share > > * @failure : VPU execution result status, 0: success, > > others: fail > > @@ -26,9 +27,11 @@ struct mtk_vcodec_ctx; > > * @dev : platform device of VPU > > * @wq : wait queue to wait VPU message ack > > * @handler : ipi handler for each decoder > > + * @codec_type : used codec type to separate different codecs > > */ > > struct vdec_vpu_inst { > > int id; > > + int core_id; > > void *vsi; > > int32_t failure; > > uint32_t inst_addr; > > @@ -38,6 +41,7 @@ struct vdec_vpu_inst { > > struct mtk_vcodec_ctx *ctx; > > wait_queue_head_t wq; > > mtk_vcodec_ipi_handler handler; > > + unsigned int codec_type; > > }; > > > > /** > >
On Wed, 1 Sept 2021 at 05:32, Yunfei Dong <yunfei.dong@mediatek.com> wrote: > > This series adds support for multi hardware decode into mtk-vcodec, by first > adding component framework to manage each hardware information: interrupt, > clock, register bases and power. Secondly add core thread to deal with core > hardware message, at the same time, add msg queue for different hardware > share messages. Lastly, the architecture of different specs are not the same, > using specs type to separate them. > > This series has been tested with both MT8183 and MT8173. Decoding was working > for both chips. > > Patches 1~3 rewrite get register bases and power on/off interface. > > Patch 4 add component framework to support multi hardware. > > Patch 5 separate video encoder and decoder document > > Patches 6-15 add interfaces to support core hardware. > ---- > This patch dependents on : "media: mtk-vcodec: support for MT8183 decoder"[1] and > "Mediatek MT8192 clock support"[2]. > > 1: Multi hardware decode is based on stateless decoder, MT8183 is the first time > to add stateless decoder. Otherwise it will cause conflict. This patch will be > accepted in 5.15[1]. > > 2: The definition of decoder clocks are in mt8192-clk.h, this patch already in clk tree[2]. > > [1]https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > [2]https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next&id=f35f1a23e0e12e3173e9e9dedbc150d139027189 > ---- > Changes compared with v5: > -Add decoder hardware block diagram for patch 13/15 > The discussion on v5 was still on-going, so sending this v6 is not helpful. The context for v5's discussion is now harder to find. Please avoid sending a new version without properly discussing all the feedback, and without reaching consensus. This is very important, please keep it in mind. Specifically, the feedback on v5 was NAK, with the request to avoid using any async framework, and instead try to find a simpler solution. For instance, you can model things with a bus-like pattern, which ties all the devices together, under a parent node. This pattern is common in the kernel, the parent node can use of_platform_populate or similar (git grep of_platform_populate, you will see plenty of examples). You will still have to do some work to have the proper regs resources, but this is doable. Each child is a device, so it can have its own resources (clocks, interrupts, iommus). You don't need any async framework. vcodec_dec: vcodec_dec@16000000 { compatible = "mediatek,mt8192-vcodec-dec"; reg = <something>; mediatek,scp = <&scp>; iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; vcodec_lat@0x10000 { compatible = "mediatek,mtk-vcodec-lat"; reg = <0x10000 0x800>; /* VDEC_MISC */ interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; // etc }; vcodec_core@0x25000 { compatible = "mediatek,mtk-vcodec-core"; reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */ interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; // etc }; }; Thanks, Ezequiel
Hi Ezequiel, Thanks for your suggestion. On Thu, 2021-09-02 at 13:30 -0300, Ezequiel Garcia wrote: > On Wed, 1 Sept 2021 at 05:32, Yunfei Dong <yunfei.dong@mediatek.com> > wrote: > > > > This series adds support for multi hardware decode into mtk-vcodec, > > by first > > adding component framework to manage each hardware information: > > interrupt, > > clock, register bases and power. Secondly add core thread to deal > > with core > > hardware message, at the same time, add msg queue for different > > hardware > > share messages. Lastly, the architecture of different specs are not > > the same, > > using specs type to separate them. > > > > This series has been tested with both MT8183 and MT8173. Decoding > > was working > > for both chips. > > > > Patches 1~3 rewrite get register bases and power on/off interface. > > > > Patch 4 add component framework to support multi hardware. > > > > Patch 5 separate video encoder and decoder document > > > > Patches 6-15 add interfaces to support core hardware. > > ---- > > This patch dependents on : "media: mtk-vcodec: support for MT8183 > > decoder"[1] and > > "Mediatek MT8192 clock support"[2]. > > > > 1: Multi hardware decode is based on stateless decoder, MT8183 is > > the first time > > to add stateless decoder. Otherwise it will cause conflict. This > > patch will be > > accepted in 5.15[1]. > > > > 2: The definition of decoder clocks are in mt8192-clk.h, this patch > > already in clk tree[2]. > > > > [1] > > https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > > [2] > > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next&id=f35f1a23e0e12e3173e9e9dedbc150d139027189 > > ---- > > Changes compared with v5: > > -Add decoder hardware block diagram for patch 13/15 > > > > > The discussion on v5 was still on-going, so sending this v6 > is not helpful. The context for v5's discussion is now harder to > find. > > Please avoid sending a new version without properly > discussing all the feedback, and without reaching consensus. > This is very important, please keep it in mind. > Thanks for your remind, I will keep this patch until get the solution. > Specifically, the feedback on v5 was NAK, with the request to avoid > using any async framework, and instead try to find a simpler > solution. > > For instance, you can model things with a bus-like pattern, > which ties all the devices together, under a parent node. > This pattern is common in the kernel, the parent > node can use of_platform_populate or similar > (git grep of_platform_populate, you will see plenty of examples). > > You will still have to do some work to have the proper > regs resources, but this is doable. Each child is a device, > so it can have its own resources (clocks, interrupts, iommus). > > You don't need any async framework. > If put the lat and core node in the parent node, need to check the below things after discussed with iommu owner. If putting the iommus property in the child node, then is the child device a standard platform device? The iommu registe like this: ret = bus_set_iommu(&platform_bus_type, &mtk_iommu_ops); It expect the consumer is a standard platform device. otherwise it could not enter to the iommu of_xlate.) Thanks Yunfei Dong > vcodec_dec: vcodec_dec@16000000 { > compatible = "mediatek,mt8192-vcodec-dec"; > reg = <something>; > mediatek,scp = <&scp>; > iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > vcodec_lat@0x10000 { > compatible = "mediatek,mtk-vcodec-lat"; > reg = <0x10000 0x800>; /* VDEC_MISC */ > interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > // etc > }; > > vcodec_core@0x25000 { > compatible = "mediatek,mtk-vcodec-core"; > reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */ > interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > // etc > }; > }; > > Thanks, > Ezequiel
On Fri, Sep 3, 2021 at 12:31 AM Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > > On Wed, 1 Sept 2021 at 05:32, Yunfei Dong <yunfei.dong@mediatek.com> wrote: > > > > This series adds support for multi hardware decode into mtk-vcodec, by first > > adding component framework to manage each hardware information: interrupt, > > clock, register bases and power. Secondly add core thread to deal with core > > hardware message, at the same time, add msg queue for different hardware > > share messages. Lastly, the architecture of different specs are not the same, > > using specs type to separate them. > > > > This series has been tested with both MT8183 and MT8173. Decoding was working > > for both chips. > > > > Patches 1~3 rewrite get register bases and power on/off interface. > > > > Patch 4 add component framework to support multi hardware. > > > > Patch 5 separate video encoder and decoder document > > > > Patches 6-15 add interfaces to support core hardware. > > ---- > > This patch dependents on : "media: mtk-vcodec: support for MT8183 decoder"[1] and > > "Mediatek MT8192 clock support"[2]. > > > > 1: Multi hardware decode is based on stateless decoder, MT8183 is the first time > > to add stateless decoder. Otherwise it will cause conflict. This patch will be > > accepted in 5.15[1]. > > > > 2: The definition of decoder clocks are in mt8192-clk.h, this patch already in clk tree[2]. > > > > [1]https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > > [2]https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next&id=f35f1a23e0e12e3173e9e9dedbc150d139027189 > > ---- > > Changes compared with v5: > > -Add decoder hardware block diagram for patch 13/15 > > > > > The discussion on v5 was still on-going, so sending this v6 > is not helpful. The context for v5's discussion is now harder to find. > > Please avoid sending a new version without properly > discussing all the feedback, and without reaching consensus. > This is very important, please keep it in mind. > > Specifically, the feedback on v5 was NAK, with the request to avoid > using any async framework, and instead try to find a simpler solution. > > For instance, you can model things with a bus-like pattern, > which ties all the devices together, under a parent node. > This pattern is common in the kernel, the parent > node can use of_platform_populate or similar > (git grep of_platform_populate, you will see plenty of examples). > > You will still have to do some work to have the proper > regs resources, but this is doable. Each child is a device, > so it can have its own resources (clocks, interrupts, iommus). AFAIK of_platform_populate doesn't guarantee the drivers actually probed (modules loaded late, missing modules, deferred probe, etc.), only that the devices are created, so you still need some sort of (async) mechanism to wait for the subdevices to be in operational state. Most of the examples using of_platform_populate are there to ensure that the parent device is operational before creating the sub-devices, not the other way around. ChenYu > You don't need any async framework. > > vcodec_dec: vcodec_dec@16000000 { > compatible = "mediatek,mt8192-vcodec-dec"; > reg = <something>; > mediatek,scp = <&scp>; > iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > vcodec_lat@0x10000 { > compatible = "mediatek,mtk-vcodec-lat"; > reg = <0x10000 0x800>; /* VDEC_MISC */ > interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > // etc > }; > > vcodec_core@0x25000 { > compatible = "mediatek,mtk-vcodec-core"; > reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */ > interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > // etc > }; > }; > > Thanks, > Ezequiel > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On 02.09.21 08:05, yunfei.dong@mediatek.com wrote: > On Wed, 2021-09-01 at 14:17 +0200, Dafna Hirschfeld wrote: > Hi Dafna, > > Thanks for your suggestion. >> Hi >> >> On 01.09.21 10:32, Yunfei Dong wrote: >>> There are just one core thread, in order to separeate different >>> hardware, using codec type to separeate it in scp driver. >> >> this code seems to relate to the vpu driver not the scp driver. >> Is there a corresponding code added to the vpu driver that test the >> codec_type? >> > Vpu is video processor unit, used to connect with micro processor. > In mt8173: vdec_vpu_if.c -> mtk_vpu.c -> micro processor > In mt8192/mt8183: vdec_vpu_if.c -> mtk_scp.c ->micro processor > > This init/dec start/dec_end interfaces are the same for vpu and scp. >>> >>> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> >>> --- >>> .../media/platform/mtk-vcodec/vdec_ipi_msg.h | 12 ++++--- >>> .../media/platform/mtk-vcodec/vdec_vpu_if.c | 34 >>> ++++++++++++++++--- >>> .../media/platform/mtk-vcodec/vdec_vpu_if.h | 4 +++ >>> 3 files changed, 41 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h >>> b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h >>> index 9d8079c4f976..c488f0c40190 100644 >>> --- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h >>> +++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h >>> @@ -35,6 +35,8 @@ enum vdec_ipi_msgid { >>> * @msg_id : vdec_ipi_msgid >>> * @vpu_inst_addr : VPU decoder instance address. Used if ABI >>> version < 2. >>> * @inst_id : instance ID. Used if the ABI version >= 2. >>> + * @codec_type : Codec fourcc >>> + * @reserved : reserved param >>> */ >>> struct vdec_ap_ipi_cmd { >>> uint32_t msg_id; >>> @@ -42,6 +44,8 @@ struct vdec_ap_ipi_cmd { >>> uint32_t vpu_inst_addr; >>> uint32_t inst_id; >>> }; >>> + uint32_t codec_type; >>> + uint32_t reserved; >>> }; >>> >>> /** >>> @@ -59,12 +63,12 @@ struct vdec_vpu_ipi_ack { >>> /** >>> * struct vdec_ap_ipi_init - for AP_IPIMSG_DEC_INIT >>> * @msg_id : AP_IPIMSG_DEC_INIT >>> - * @reserved : Reserved field >>> + * @codec_type : Codec fourcc >>> * @ap_inst_addr : AP video decoder instance address >>> */ >>> struct vdec_ap_ipi_init { >>> uint32_t msg_id; >>> - uint32_t reserved; >>> + uint32_t codec_type; >>> uint64_t ap_inst_addr; >>> }; >>> >>> @@ -77,7 +81,7 @@ struct vdec_ap_ipi_init { >>> * H264 decoder [0]:buf_sz [1]:nal_start >>> * VP8 decoder [0]:width/height >>> * VP9 decoder [0]:profile, [1][2] width/height >>> - * @reserved : Reserved field >>> + * @codec_type : Codec fourcc >>> */ >>> struct vdec_ap_ipi_dec_start { >>> uint32_t msg_id; >>> @@ -86,7 +90,7 @@ struct vdec_ap_ipi_dec_start { >>> uint32_t inst_id; >>> }; >>> uint32_t data[3]; >>> - uint32_t reserved; >>> + uint32_t codec_type; >>> }; >>> >>> /** >>> diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c >>> b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c >>> index bfd8e87dceff..c84fac52fe26 100644 >>> --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c >>> +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c >>> @@ -100,18 +100,29 @@ static void vpu_dec_ipi_handler(void *data, >>> unsigned int len, void *priv) >>> >>> static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void >>> *msg, int len) >>> { >>> - int err; >>> + int err, id, msgid; >>> >>> - mtk_vcodec_debug(vpu, "id=%X", *(uint32_t *)msg); >>> + msgid = *(uint32_t *)msg; >>> + mtk_vcodec_debug(vpu, "id=%X", msgid); >>> >>> vpu->failure = 0; >>> vpu->signaled = 0; >>> >>> - err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, vpu- >>>> id, msg, >>> + if (vpu->ctx->dev->vdec_pdata->hw_arch == >>> MTK_VDEC_LAT_SINGLE_CORE) { >>> + if (msgid == AP_IPIMSG_DEC_CORE || >>> + msgid == AP_IPIMSG_DEC_CORE_END) >>> + id = vpu->core_id; >>> + else >>> + id = vpu->id; >>> + } else { >>> + id = vpu->id; >>> + } >>> + >>> + err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, id, >>> msg, >>> len, 2000); >> >> so >>> if (err) { >>> mtk_vcodec_err(vpu, "send fail vpu_id=%d msg_id=%X >>> status=%d", >>> - vpu->id, *(uint32_t *)msg, err); >>> + id, msgid, err); >>> return err; >>> } >>> >>> @@ -131,6 +142,7 @@ static int vcodec_send_ap_ipi(struct >>> vdec_vpu_inst *vpu, unsigned int msg_id) >>> msg.vpu_inst_addr = vpu->inst_addr; >>> else >>> msg.inst_id = vpu->inst_id; >>> + msg.codec_type = vpu->codec_type; >>> >>> err = vcodec_vpu_send_msg(vpu, &msg, sizeof(msg)); >>> mtk_vcodec_debug(vpu, "- id=%X ret=%d", msg_id, err); >>> @@ -149,14 +161,25 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu) >>> >>> err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, >>> vpu->id, >>> vpu->handler, "vdec", NULL); >>> - if (err != 0) { >>> + if (err) { >> >> could be nice to send a patch with other such fixes, >> anyway it is better to send unrelated fixes in a separate patch >> > will fix in next patch. >>> mtk_vcodec_err(vpu, "vpu_ipi_register fail status=%d", >>> err); >>> return err; >>> } >>> >>> + if (vpu->ctx->dev->vdec_pdata->hw_arch == >>> MTK_VDEC_LAT_SINGLE_CORE) { >>> + err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev- >>>> fw_handler, >>> + vpu->core_id, vpu->handler, >>> + "vdec", NULL); >>> + if (err) { >>> + mtk_vcodec_err(vpu, "vpu_ipi_register core fail >>> status=%d", err); >>> + return err; >>> + } >>> + } >>> + >>> memset(&msg, 0, sizeof(msg)); >>> msg.msg_id = AP_IPIMSG_DEC_INIT; >>> msg.ap_inst_addr = (unsigned long)vpu; >>> + msg.codec_type = vpu->codec_type; >>> >>> mtk_vcodec_debug(vpu, "vdec_inst=%p", vpu); >>> >>> @@ -187,6 +210,7 @@ int vpu_dec_start(struct vdec_vpu_inst *vpu, >>> uint32_t *data, unsigned int len) >>> >>> for (i = 0; i < len; i++) >>> msg.data[i] = data[i]; >>> + msg.codec_type = vpu->codec_type; >> >> I don't see where is the vpu->codec_type initialzied >> > This patch just add interface to support core hardware decode, in next > serial patches based on these will used codec type to separate after > these base patches are stable. Then why not send it on the same series? Thanks, Dafna >> Thanks, >> Dafna >> > Thanks > Yunfei Dong >>> >>> err = vcodec_vpu_send_msg(vpu, (void *)&msg, sizeof(msg)); >>> mtk_vcodec_debug(vpu, "- ret=%d", err); >>> diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h >>> b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h >>> index ae24b75d1649..802660770a87 100644 >>> --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h >>> +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h >>> @@ -14,6 +14,7 @@ struct mtk_vcodec_ctx; >>> /** >>> * struct vdec_vpu_inst - VPU instance for video codec >>> * @id : ipi msg id for each decoder >>> + * @core_id : core id used to separate different hardware >>> * @vsi : driver structure allocated by VPU side and >>> shared to AP side >>> * for control and info share >>> * @failure : VPU execution result status, 0: success, >>> others: fail >>> @@ -26,9 +27,11 @@ struct mtk_vcodec_ctx; >>> * @dev : platform device of VPU >>> * @wq : wait queue to wait VPU message ack >>> * @handler : ipi handler for each decoder >>> + * @codec_type : used codec type to separate different codecs >>> */ >>> struct vdec_vpu_inst { >>> int id; >>> + int core_id; >>> void *vsi; >>> int32_t failure; >>> uint32_t inst_addr; >>> @@ -38,6 +41,7 @@ struct vdec_vpu_inst { >>> struct mtk_vcodec_ctx *ctx; >>> wait_queue_head_t wq; >>> mtk_vcodec_ipi_handler handler; >>> + unsigned int codec_type; >>> }; >>> >>> /** >>>
Hi Danfa, Thanks for your suggestion. On Fri, 2021-09-03 at 14:42 +0200, Dafna Hirschfeld wrote: > > On 02.09.21 08:05, yunfei.dong@mediatek.com wrote: > > On Wed, 2021-09-01 at 14:17 +0200, Dafna Hirschfeld wrote: > > Hi Dafna, > > > > Thanks for your suggestion. > > > Hi > > > > > > On 01.09.21 10:32, Yunfei Dong wrote: > > > > There are just one core thread, in order to separeate different > > > > hardware, using codec type to separeate it in scp driver. > > > > > > this code seems to relate to the vpu driver not the scp driver. > > > Is there a corresponding code added to the vpu driver that test > > > the > > > codec_type? > > > > > > > Vpu is video processor unit, used to connect with micro processor. > > In mt8173: vdec_vpu_if.c -> mtk_vpu.c -> micro processor > > In mt8192/mt8183: vdec_vpu_if.c -> mtk_scp.c ->micro processor > > > > This init/dec start/dec_end interfaces are the same for vpu and > > scp. > > > > > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > > > --- > > > > .../media/platform/mtk-vcodec/vdec_ipi_msg.h | 12 ++++--- > > > > .../media/platform/mtk-vcodec/vdec_vpu_if.c | 34 > > > > ++++++++++++++++--- > > > > .../media/platform/mtk-vcodec/vdec_vpu_if.h | 4 +++ > > > > 3 files changed, 41 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > > > b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > > > index 9d8079c4f976..c488f0c40190 100644 > > > > --- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h > > > > @@ -35,6 +35,8 @@ enum vdec_ipi_msgid { > > > > * @msg_id : vdec_ipi_msgid > > > > * @vpu_inst_addr : VPU decoder instance address. Used if > > > > ABI > > > > version < 2. > > > > * @inst_id : instance ID. Used if the ABI version >= 2. > > > > + * @codec_type : Codec fourcc > > > > + * @reserved : reserved param > > > > */ > > > > struct vdec_ap_ipi_cmd { > > > > uint32_t msg_id; > > > > @@ -42,6 +44,8 @@ struct vdec_ap_ipi_cmd { > > > > uint32_t vpu_inst_addr; > > > > uint32_t inst_id; > > > > }; > > > > + uint32_t codec_type; > > > > + uint32_t reserved; > > > > }; > > > > > > > > /** > > > > @@ -59,12 +63,12 @@ struct vdec_vpu_ipi_ack { > > > > /** > > > > * struct vdec_ap_ipi_init - for AP_IPIMSG_DEC_INIT > > > > * @msg_id : AP_IPIMSG_DEC_INIT > > > > - * @reserved : Reserved field > > > > + * @codec_type : Codec fourcc > > > > * @ap_inst_addr : AP video decoder instance address > > > > */ > > > > struct vdec_ap_ipi_init { > > > > uint32_t msg_id; > > > > - uint32_t reserved; > > > > + uint32_t codec_type; > > > > uint64_t ap_inst_addr; > > > > }; > > > > > > > > @@ -77,7 +81,7 @@ struct vdec_ap_ipi_init { > > > > * H264 decoder [0]:buf_sz [1]:nal_start > > > > * VP8 decoder [0]:width/height > > > > * VP9 decoder [0]:profile, [1][2] width/height > > > > - * @reserved : Reserved field > > > > + * @codec_type : Codec fourcc > > > > */ > > > > struct vdec_ap_ipi_dec_start { > > > > uint32_t msg_id; > > > > @@ -86,7 +90,7 @@ struct vdec_ap_ipi_dec_start { > > > > uint32_t inst_id; > > > > }; > > > > uint32_t data[3]; > > > > - uint32_t reserved; > > > > + uint32_t codec_type; > > > > }; > > > > > > > > /** > > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > > > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > > > index bfd8e87dceff..c84fac52fe26 100644 > > > > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c > > > > @@ -100,18 +100,29 @@ static void vpu_dec_ipi_handler(void > > > > *data, > > > > unsigned int len, void *priv) > > > > > > > > static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, > > > > void > > > > *msg, int len) > > > > { > > > > - int err; > > > > + int err, id, msgid; > > > > > > > > - mtk_vcodec_debug(vpu, "id=%X", *(uint32_t *)msg); > > > > + msgid = *(uint32_t *)msg; > > > > + mtk_vcodec_debug(vpu, "id=%X", msgid); > > > > > > > > vpu->failure = 0; > > > > vpu->signaled = 0; > > > > > > > > - err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, > > > > vpu- > > > > > id, msg, > > > > > > > > + if (vpu->ctx->dev->vdec_pdata->hw_arch == > > > > MTK_VDEC_LAT_SINGLE_CORE) { > > > > + if (msgid == AP_IPIMSG_DEC_CORE || > > > > + msgid == AP_IPIMSG_DEC_CORE_END) > > > > + id = vpu->core_id; > > > > + else > > > > + id = vpu->id; > > > > + } else { > > > > + id = vpu->id; > > > > + } > > > > + > > > > + err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, > > > > id, > > > > msg, > > > > len, 2000); > > > > > > so > > > > if (err) { > > > > mtk_vcodec_err(vpu, "send fail vpu_id=%d > > > > msg_id=%X > > > > status=%d", > > > > - vpu->id, *(uint32_t *)msg, err); > > > > + id, msgid, err); > > > > return err; > > > > } > > > > > > > > @@ -131,6 +142,7 @@ static int vcodec_send_ap_ipi(struct > > > > vdec_vpu_inst *vpu, unsigned int msg_id) > > > > msg.vpu_inst_addr = vpu->inst_addr; > > > > else > > > > msg.inst_id = vpu->inst_id; > > > > + msg.codec_type = vpu->codec_type; > > > > > > > > err = vcodec_vpu_send_msg(vpu, &msg, sizeof(msg)); > > > > mtk_vcodec_debug(vpu, "- id=%X ret=%d", msg_id, err); > > > > @@ -149,14 +161,25 @@ int vpu_dec_init(struct vdec_vpu_inst > > > > *vpu) > > > > > > > > err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev- > > > > >fw_handler, > > > > vpu->id, > > > > vpu->handler, "vdec", > > > > NULL); > > > > - if (err != 0) { > > > > + if (err) { > > > > > > could be nice to send a patch with other such fixes, > > > anyway it is better to send unrelated fixes in a separate patch > > > > > > > will fix in next patch. > > > > mtk_vcodec_err(vpu, "vpu_ipi_register fail > > > > status=%d", > > > > err); > > > > return err; > > > > } > > > > > > > > + if (vpu->ctx->dev->vdec_pdata->hw_arch == > > > > MTK_VDEC_LAT_SINGLE_CORE) { > > > > + err = mtk_vcodec_fw_ipi_register(vpu->ctx->dev- > > > > > fw_handler, > > > > > > > > + vpu->core_id, vpu- > > > > >handler, > > > > + "vdec", NULL); > > > > + if (err) { > > > > + mtk_vcodec_err(vpu, "vpu_ipi_register > > > > core fail > > > > status=%d", err); > > > > + return err; > > > > + } > > > > + } > > > > + > > > > memset(&msg, 0, sizeof(msg)); > > > > msg.msg_id = AP_IPIMSG_DEC_INIT; > > > > msg.ap_inst_addr = (unsigned long)vpu; > > > > + msg.codec_type = vpu->codec_type; > > > > > > > > mtk_vcodec_debug(vpu, "vdec_inst=%p", vpu); > > > > > > > > @@ -187,6 +210,7 @@ int vpu_dec_start(struct vdec_vpu_inst > > > > *vpu, > > > > uint32_t *data, unsigned int len) > > > > > > > > for (i = 0; i < len; i++) > > > > msg.data[i] = data[i]; > > > > + msg.codec_type = vpu->codec_type; > > > > > > I don't see where is the vpu->codec_type initialzied > > > > > > > This patch just add interface to support core hardware decode, in > > next > > serial patches based on these will used codec type to separate > > after > > these base patches are stable. > > Then why not send it on the same series? > For this patch is the common part, not only one spec(h264/vp9/av1) use it. And these specs will send in different patch series, in order to avoid patch dependency, so add this patch in this common series. > Thanks, > Dafna > Thanks Yunfei Dong > > > Thanks, > > > Dafna > > > > > > > Thanks > > Yunfei Dong > > > > > > > > err = vcodec_vpu_send_msg(vpu, (void *)&msg, > > > > sizeof(msg)); > > > > mtk_vcodec_debug(vpu, "- ret=%d", err); > > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > > > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > > > index ae24b75d1649..802660770a87 100644 > > > > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h > > > > @@ -14,6 +14,7 @@ struct mtk_vcodec_ctx; > > > > /** > > > > * struct vdec_vpu_inst - VPU instance for video codec > > > > * @id : ipi msg id for each decoder > > > > + * @core_id : core id used to separate different hardware > > > > * @vsi : driver structure allocated by VPU side and > > > > shared to AP side > > > > * for control and info share > > > > * @failure : VPU execution result status, 0: success, > > > > others: fail > > > > @@ -26,9 +27,11 @@ struct mtk_vcodec_ctx; > > > > * @dev : platform device of VPU > > > > * @wq : wait queue to wait VPU message ack > > > > * @handler : ipi handler for each decoder > > > > + * @codec_type : used codec type to separate different > > > > codecs > > > > */ > > > > struct vdec_vpu_inst { > > > > int id; > > > > + int core_id; > > > > void *vsi; > > > > int32_t failure; > > > > uint32_t inst_addr; > > > > @@ -38,6 +41,7 @@ struct vdec_vpu_inst { > > > > struct mtk_vcodec_ctx *ctx; > > > > wait_queue_head_t wq; > > > > mtk_vcodec_ipi_handler handler; > > > > + unsigned int codec_type; > > > > }; > > > > > > > > /** > > > >
Hi Ezequiel, On Fri, 2021-09-03 at 11:08 +0800, yunfei.dong@mediatek.com wrote: > Hi Ezequiel, > > Thanks for your suggestion. > On Thu, 2021-09-02 at 13:30 -0300, Ezequiel Garcia wrote: > > On Wed, 1 Sept 2021 at 05:32, Yunfei Dong <yunfei.dong@mediatek.com > > > > > wrote: > > > > > > This series adds support for multi hardware decode into mtk- > > > vcodec, > > > by first > > > adding component framework to manage each hardware information: > > > interrupt, > > > clock, register bases and power. Secondly add core thread to deal > > > with core > > > hardware message, at the same time, add msg queue for different > > > hardware > > > share messages. Lastly, the architecture of different specs are > > > not > > > the same, > > > using specs type to separate them. > > > > > > This series has been tested with both MT8183 and MT8173. Decoding > > > was working > > > for both chips. > > > > > > Patches 1~3 rewrite get register bases and power on/off > > > interface. > > > > > > Patch 4 add component framework to support multi hardware. > > > > > > Patch 5 separate video encoder and decoder document > > > > > > Patches 6-15 add interfaces to support core hardware. > > > ---- > > > This patch dependents on : "media: mtk-vcodec: support for MT8183 > > > decoder"[1] and > > > "Mediatek MT8192 clock support"[2]. > > > > > > 1: Multi hardware decode is based on stateless decoder, MT8183 is > > > the first time > > > to add stateless decoder. Otherwise it will cause conflict. This > > > patch will be > > > accepted in 5.15[1]. > > > > > > 2: The definition of decoder clocks are in mt8192-clk.h, this > > > patch > > > already in clk tree[2]. > > > > > > [1] > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > > > [2] > > > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next&id=f35f1a23e0e12e3173e9e9dedbc150d139027189 > > > ---- > > > Changes compared with v5: > > > -Add decoder hardware block diagram for patch 13/15 > > > > > > > > > The discussion on v5 was still on-going, so sending this v6 > > is not helpful. The context for v5's discussion is now harder to > > find. > > > > Please avoid sending a new version without properly > > discussing all the feedback, and without reaching consensus. > > This is very important, please keep it in mind. > > > > Thanks for your remind, I will keep this patch until get the > solution. > > > Specifically, the feedback on v5 was NAK, with the request to avoid > > using any async framework, and instead try to find a simpler > > solution. > > > > For instance, you can model things with a bus-like pattern, > > which ties all the devices together, under a parent node. > > This pattern is common in the kernel, the parent > > node can use of_platform_populate or similar > > (git grep of_platform_populate, you will see plenty of examples). > > > > You will still have to do some work to have the proper > > regs resources, but this is doable. Each child is a device, > > so it can have its own resources (clocks, interrupts, iommus). > > > > You don't need any async framework. > > > Thanks for your suggestion very much, and there are several actions need to check. 1: The iommu register like this: ret = bus_set_iommu(&platform_bus_type, &mtk_iommu_ops); It expect the consumer is a standard platform device. otherwise it could not enter to the iommu of_xlate.) So if putting the iommus property in the child node, all the child device need to registered as platform device. 2: For the interrupt in each child node, but the logical processing in parent part. Child and parent need to send message for each other. In order to control clk/power/irq for multi instance, need send message to child to separate different hardware; child also need send message to parent when get interrupt. 3: About Chen-Yu's mail, do you have any advice? Do you have any suggestion about these two scenarios? I'm very happy to get your reply. Thanks Yunfei Dong > > vcodec_dec: vcodec_dec@16000000 { > > compatible = "mediatek,mt8192-vcodec-dec"; > > reg = <something>; > > mediatek,scp = <&scp>; > > iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > vcodec_lat@0x10000 { > > compatible = "mediatek,mtk-vcodec-lat"; > > reg = <0x10000 0x800>; /* VDEC_MISC */ > > interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > // etc > > }; > > > > vcodec_core@0x25000 { > > compatible = "mediatek,mtk-vcodec-core"; > > reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */ > > interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > // etc > > }; > > }; > > > > Thanks, > > Ezequiel
Hi Ezequiel, Could you please help to give some feedback when you are free for iommu limitation? According to google's suggestion, it's better not to use v4l2 async also. If there are no further comments, I don't have other choice for this architecture. Thanks, Yunfei Dong On Tue, 2021-09-14 at 20:16 +0800, yunfei.dong@mediatek.com wrote: > Hi Ezequiel, > > On Fri, 2021-09-03 at 11:08 +0800, yunfei.dong@mediatek.com wrote: > > Hi Ezequiel, > > > > Thanks for your suggestion. > > On Thu, 2021-09-02 at 13:30 -0300, Ezequiel Garcia wrote: > > > On Wed, 1 Sept 2021 at 05:32, Yunfei Dong < > > > yunfei.dong@mediatek.com > > > > > > > > > > wrote: > > > > > > > > This series adds support for multi hardware decode into mtk- > > > > vcodec, > > > > by first > > > > adding component framework to manage each hardware information: > > > > interrupt, > > > > clock, register bases and power. Secondly add core thread to > > > > deal > > > > with core > > > > hardware message, at the same time, add msg queue for different > > > > hardware > > > > share messages. Lastly, the architecture of different specs are > > > > not > > > > the same, > > > > using specs type to separate them. > > > > > > > > This series has been tested with both MT8183 and MT8173. > > > > Decoding > > > > was working > > > > for both chips. > > > > > > > > Patches 1~3 rewrite get register bases and power on/off > > > > interface. > > > > > > > > Patch 4 add component framework to support multi hardware. > > > > > > > > Patch 5 separate video encoder and decoder document > > > > > > > > Patches 6-15 add interfaces to support core hardware. > > > > ---- > > > > This patch dependents on : "media: mtk-vcodec: support for > > > > MT8183 > > > > decoder"[1] and > > > > "Mediatek MT8192 clock support"[2]. > > > > > > > > 1: Multi hardware decode is based on stateless decoder, MT8183 > > > > is > > > > the first time > > > > to add stateless decoder. Otherwise it will cause conflict. > > > > This > > > > patch will be > > > > accepted in 5.15[1]. > > > > > > > > 2: The definition of decoder clocks are in mt8192-clk.h, this > > > > patch > > > > already in clk tree[2]. > > > > > > > > [1] > > > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > > > > [2] > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next&id=f35f1a23e0e12e3173e9e9dedbc150d139027189 > > > > ---- > > > > Changes compared with v5: > > > > -Add decoder hardware block diagram for patch 13/15 > > > > > > > > > > > > > The discussion on v5 was still on-going, so sending this v6 > > > is not helpful. The context for v5's discussion is now harder to > > > find. > > > > > > Please avoid sending a new version without properly > > > discussing all the feedback, and without reaching consensus. > > > This is very important, please keep it in mind. > > > > > > > Thanks for your remind, I will keep this patch until get the > > solution. > > > > > Specifically, the feedback on v5 was NAK, with the request to > > > avoid > > > using any async framework, and instead try to find a simpler > > > solution. > > > > > > For instance, you can model things with a bus-like pattern, > > > which ties all the devices together, under a parent node. > > > This pattern is common in the kernel, the parent > > > node can use of_platform_populate or similar > > > (git grep of_platform_populate, you will see plenty of examples). > > > > > > You will still have to do some work to have the proper > > > regs resources, but this is doable. Each child is a device, > > > so it can have its own resources (clocks, interrupts, iommus). > > > > > > You don't need any async framework. > > > > > Thanks for your suggestion very much, and there are several actions > need to check. > > 1: The iommu register like this: > ret = bus_set_iommu(&platform_bus_type, > &mtk_iommu_ops); > It expect the consumer is a standard platform device. > otherwise it > could not enter to the iommu of_xlate.) > > So if putting the iommus property in the child node, all the child > device need to registered as platform device. > > 2: For the interrupt in each child node, but the logical processing > in > parent part. Child and parent need to send message for each other. In > order to control clk/power/irq for multi instance, need send message > to > child to separate different hardware; child also need send message to > parent when get interrupt. > > 3: About Chen-Yu's mail, do you have any advice? > > Do you have any suggestion about these two scenarios? > I'm very happy to get your reply. > > Thanks > Yunfei Dong > > > > vcodec_dec: vcodec_dec@16000000 { > > > compatible = "mediatek,mt8192-vcodec-dec"; > > > reg = <something>; > > > mediatek,scp = <&scp>; > > > iommus = <&iommu0 M4U_PORT_L4_VDEC_MC_EXT>; > > > dma-ranges = <0x1 0x0 0x0 0x40000000 0x0 0xfff00000>; > > > > > > vcodec_lat@0x10000 { > > > compatible = "mediatek,mtk-vcodec-lat"; > > > reg = <0x10000 0x800>; /* VDEC_MISC */ > > > interrupts = <GIC_SPI 426 IRQ_TYPE_LEVEL_HIGH 0>; > > > // etc > > > }; > > > > > > vcodec_core@0x25000 { > > > compatible = "mediatek,mtk-vcodec-core"; > > > reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */ > > > interrupts = <GIC_SPI 425 IRQ_TYPE_LEVEL_HIGH 0>; > > > // etc > > > }; > > > }; > > > > > > Thanks, > > > Ezequiel
On Sun, 26 Sept 2021 at 05:27, yunfei.dong@mediatek.com <yunfei.dong@mediatek.com> wrote: > > Hi Ezequiel, > > Could you please help to give some feedback when you are free for iommu > limitation? > How about you work on the architecture I originally suggested? As the saying goes, talk is cheap, show us the code. So let's see the code and let's discuss the limitations with the code. > According to google's suggestion, it's better not to use v4l2 async > also. Hum? I haven't seen such objection on the mailing list. Thanks, Ezequiel
Hi Yunfei, > This patch dependents on : "media: mtk-vcodec: support for MT8183 decoder"[1] and > "Mediatek MT8192 clock support"[2]. > > 1: Multi hardware decode is based on stateless decoder, MT8183 is the first time > to add stateless decoder. Otherwise it will cause conflict. This patch will be > accepted in 5.15[1]. Just a few basic questions. What kind of "conflict" are you expecting here? Are you referring to kernel "5.15" here? Probably not. If yes, then that sounds strange to me considering our current kernel uprev plans and current kernel used for MT8183. > 2: The definition of decoder clocks are in mt8192-clk.h, this patch already in clk tree[2]. > > [1]https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 This link seems to be no longer available. > [2]https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next&id=f35f1a23e0e12e3173e9e9dedbc150d139027189
Hi Ezequiel, Thank you for reviewing these series from Yunfei! This series is one of the main obstacles for us at the moment for MTK so please continue to help & support reviewing this series. > > According to google's suggestion, it's better not to use v4l2 async > > also. > > Hum? I haven't seen such objection on the mailing list. Maybe coming from Tzung-Bi? Yunfei, please let us know. Steve
> > [1]https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > This link seems to be no longer available. It is probably because the default state is "Action Required", and this series was accepted recently afterwards. I can see the series with the below link. https://patchwork.linuxtv.org/project/linux-media/list/?series=5826&state=* Sorry for the confusion. Steve
On Tue, Sep 28, 2021 at 2:02 AM Steve Cho <stevecho@chromium.org> wrote: > > Hi Ezequiel, > > Thank you for reviewing these series from Yunfei! > This series is one of the main obstacles for us at the moment for MTK > so please continue to help & support reviewing this series. > > > > According to google's suggestion, it's better not to use v4l2 async > > > also. > > > > Hum? I haven't seen such objection on the mailing list. > Maybe coming from Tzung-Bi? > Yunfei, please let us know. I do object to using V4L2 async. It's designed for independent components of media pipelines, handled by multiple different drivers and also modelled as V4L2 subdevices. We don't have anything like that here. How about just open coding something trivial that only fits the needs of this specific driver? I think it would be as simple as a linked list and registering the V4L2 devices only after all the nodes probe. Best regards, Tomasz
Le 01/09/2021 à 10:32, Yunfei Dong a écrit : > Use the dma_set_mask_and_coherent helper to set vdec > DMA bit mask to support 34bits iova space(16GB) that > the mt8192 iommu HW support. > > Whole the iova range separate to 0~4G/4G~8G/8G~12G/12G~16G, > regarding which iova range VDEC actually locate, it > depends on the dma-ranges property of vdec dtsi node. > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > index 002352fcf8de..1a8d9308327d 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > @@ -417,6 +417,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev) > } > } > > + if (of_get_property(pdev->dev.of_node, "dma-ranges", NULL)) > + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(34)); > + Hi Yunfei, Does all SoC support 34bits iova space ? If not you need to also check SoC version before setting dma mask. Regards, Benjamin > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > mutex_init(&dev->dec_mutex[i]); > spin_lock_init(&dev->irqlock);
Hi Benjiamin, On Thu, 2021-10-07 at 13:37 +0200, Benjamin Gaignard wrote: > Le 01/09/2021 à 10:32, Yunfei Dong a écrit : > > Use the dma_set_mask_and_coherent helper to set vdec > > DMA bit mask to support 34bits iova space(16GB) that > > the mt8192 iommu HW support. > > > > Whole the iova range separate to 0~4G/4G~8G/8G~12G/12G~16G, > > regarding which iova range VDEC actually locate, it > > depends on the dma-ranges property of vdec dtsi node. > > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > index 002352fcf8de..1a8d9308327d 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > @@ -417,6 +417,9 @@ static int mtk_vcodec_probe(struct > > platform_device *pdev) > > } > > } > > > > + if (of_get_property(pdev->dev.of_node, "dma-ranges", NULL)) > > + dma_set_mask_and_coherent(&pdev->dev, > > DMA_BIT_MASK(34)); > > + > > Hi Yunfei, > > Does all SoC support 34bits iova space ? > If not you need to also check SoC version before setting dma mask. > Not all SoC support 34bits. Will add dma-ranges property in dtsi if the SoC support 34bits. like this: dma-ranges = <>; Thanks, Yunfei Dong > Regards, > Benjamin > > > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > > mutex_init(&dev->dec_mutex[i]); > > spin_lock_init(&dev->irqlock);
Hi Ezequiel, Thanks for your feedback, The driver can work well now according to your advice with of_platform_populate interface. In order to separate parent node with children node, parent node is master device, children node is component device. The master and component are registered platform device. Could you please help to review the patch again when you are free: https://patchwork.linuxtv.org/project/linux-media/cover/20211011070247.792-1-yunfei.dong@mediatek.com/ Best Regards, Yunfei Dong On Sun, 2021-09-26 at 11:51 -0300, Ezequiel Garcia wrote: > On Sun, 26 Sept 2021 at 05:27, yunfei.dong@mediatek.com > <yunfei.dong@mediatek.com> wrote: > > > > Hi Ezequiel, > > > > Could you please help to give some feedback when you are free for > > iommu > > limitation? > > > > How about you work on the architecture I originally suggested? > > As the saying goes, talk is cheap, show us the code. > So let's see the code and let's discuss the limitations > with the code. > > > According to google's suggestion, it's better not to use v4l2 async > > also. > > Hum? I haven't seen such objection on the mailing list. > > Thanks, > Ezequiel
Hi Yunfei, On Tue, 12 Oct 2021 at 22:17, yunfei.dong@mediatek.com <yunfei.dong@mediatek.com> wrote: > > Hi Ezequiel, > > Thanks for your feedback, > > The driver can work well now according to your advice with > of_platform_populate interface. > > In order to separate parent node with children node, parent node is > master device, children node is component device. > > The master and component are registered platform device. > > > Could you please help to review the patch again when you are free: > > https://patchwork.linuxtv.org/project/linux-media/cover/20211011070247.792-1-yunfei.dong@mediatek.com/ > I'm glad you managed to simplify the driver. I tried applying the patches but they don't apply on media master. Please push a branch to gitlab or github or somewhere public. Keep in mind that when you need people to review your code, it's generally good practice to try to make it easy on them. The harder you make it, the less inclined people will be to spend time on your work. Thanks, Ezequiel