mbox series

[v5,00/15] Using component framework to support multi hardware decode

Message ID 20210811025801.21597-1-yunfei.dong@mediatek.com (mailing list archive)
Headers show
Series Using component framework to support multi hardware decode | expand

Message

Yunfei Dong Aug. 11, 2021, 2:57 a.m. UTC
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. Please also accept
this patch together with [1].

2: The definition of decoder clocks are in mt8192-clk.h, need to include them in
case of build fail [2].

[1]https://patchwork.linuxtv.org/project/linux-media/list/?series=6057
[2]https://patchwork.kernel.org/project/linux-mediatek/list/?series=511175
----
Changes compared with v4:
- Fix comments for patch 4/15
  >> +     if (dev->is_comp_supported) {
  >> +             ret = mtk_vcodec_init_master(dev);
  >> +             if (ret < 0)
  >> +                     goto err_component_match;
  >> +     } else {
  >> +             platform_set_drvdata(pdev, dev);
  >> +     }
  Fix platform_set_drvdata.
- Fix build error for patch 9/15
- Add depend patch in case of error header file for patch 13/15

Changes compared with v3:
- Fix return value for patch 1/15
- Fix comments for patch 4/15
  > Looking up "mediatek,mtk-vcodec-core" to determine if it uses component framwork sounds like...
  Add prameter in pdata, for all platform will use compoent after mt8183

  >> +     if (dev->is_comp_supported) {
  >> +             ret = mtk_vcodec_init_master(dev);
  >> +             if (ret < 0)
  >> +                     goto err_component_match;
  >> +     } else {
  >> +             platform_set_drvdata(pdev, dev);
  >> +     }
  > + Has asked the same question in [1].  Why it removes the
  > +platform_set_drvdata() above?  mtk_vcodec_init_master() also calls platform_set_drvdata().
  Must call component_master_add_with_match after platform_set_drvdata for component architecture.
- Fix yaml files check fail for patch 5/15
- Fix yaml file check fail for patch 14/15

Changes compared with v1:
- Fix many comments for patch 3/14
- Remove unnecessary code for patch 4/14
- Using enum mtk_vdec_hw_count instead of magic numbers for patch 6/14
- Reconstructed get/put lat buffer for lat and core hardware for patch 7/14
- Using yaml format to instead of txt file for patch 12/14

Yunfei Dong (15):
  media: mtk-vcodec: Get numbers of register bases from DT
  media: mtk-vcodec: Align vcodec wake up interrupt interface
  media: mtk-vcodec: Refactor vcodec pm interface
  media: mtk-vcodec: Use component framework to manage each hardware
    information
  dt-bindings: media: mtk-vcodec: Separate video encoder and decoder
    dt-bindings
  media: mtk-vcodec: Use pure single core for MT8183
  media: mtk-vcodec: Add irq interface for multi hardware
  media: mtk-vcodec: Add msg queue feature for lat and core architecture
  media: mtk-vcodec: Generalize power and clock on/off interfaces
  media: mtk-vcodec: Add new interface to lock different hardware
  media: mtk-vcodec: Add core thread
  media: mtk-vcodec: Support 34bits dma address for vdec
  dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192
  media: mtk-vcodec: Add core dec and dec end ipi msg
  media: mtk-vcodec: Use codec type to separate different hardware

 .../media/mediatek,vcodec-comp-decoder.yaml   | 172 +++++++++++
 .../media/mediatek,vcodec-decoder.yaml        | 175 +++++++++++
 .../media/mediatek,vcodec-encoder.yaml        | 185 +++++++++++
 .../bindings/media/mediatek-vcodec.txt        | 130 --------
 drivers/media/platform/mtk-vcodec/Makefile    |   2 +
 .../platform/mtk-vcodec/mtk_vcodec_dec.c      |   4 +-
 .../platform/mtk-vcodec/mtk_vcodec_dec.h      |   1 +
 .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  | 276 ++++++++++++++---
 .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c   | 184 +++++++++++
 .../platform/mtk-vcodec/mtk_vcodec_dec_hw.h   |  53 ++++
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   |  98 ++++--
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h   |  13 +-
 .../mtk-vcodec/mtk_vcodec_dec_stateful.c      |   2 +
 .../mtk-vcodec/mtk_vcodec_dec_stateless.c     |   2 +
 .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  71 ++++-
 .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c  |  12 +-
 .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   |   1 -
 .../platform/mtk-vcodec/mtk_vcodec_intr.c     |  27 +-
 .../platform/mtk-vcodec/mtk_vcodec_intr.h     |   4 +-
 .../platform/mtk-vcodec/mtk_vcodec_util.c     |  87 +++++-
 .../platform/mtk-vcodec/mtk_vcodec_util.h     |   8 +-
 .../platform/mtk-vcodec/vdec/vdec_h264_if.c   |   2 +-
 .../mtk-vcodec/vdec/vdec_h264_req_if.c        |   2 +-
 .../platform/mtk-vcodec/vdec/vdec_vp8_if.c    |   2 +-
 .../platform/mtk-vcodec/vdec/vdec_vp9_if.c    |   2 +-
 .../media/platform/mtk-vcodec/vdec_drv_if.c   |  21 +-
 .../media/platform/mtk-vcodec/vdec_ipi_msg.h  |  16 +-
 .../platform/mtk-vcodec/vdec_msg_queue.c      | 290 ++++++++++++++++++
 .../platform/mtk-vcodec/vdec_msg_queue.h      | 157 ++++++++++
 .../media/platform/mtk-vcodec/vdec_vpu_if.c   |  46 ++-
 .../media/platform/mtk-vcodec/vdec_vpu_if.h   |  22 ++
 .../platform/mtk-vcodec/venc/venc_h264_if.c   |   2 +-
 .../platform/mtk-vcodec/venc/venc_vp8_if.c    |   2 +-
 33 files changed, 1792 insertions(+), 279 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-comp-decoder.yaml
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-decoder.yaml
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,vcodec-encoder.yaml
 delete mode 100644 Documentation/devicetree/bindings/media/mediatek-vcodec.txt
 create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
 create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
 create mode 100644 drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
 create mode 100644 drivers/media/platform/mtk-vcodec/vdec_msg_queue.h

Comments

Ezequiel Garcia Aug. 18, 2021, 2:11 p.m. UTC | #1
+danvet

Hi,

On Tue, 10 Aug 2021 at 23:58, 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.
>

I don't think it's a good idea to introduce the component API in the
media subsystem. It doesn't seem to be maintained, IRC there's not even
a maintainer for it, and it has some issues that were never addressed.

It would be really important to avoid it. Is it really needed in the
first place?

Thanks,
Ezequiel
Yunfei Dong Aug. 19, 2021, 7:13 a.m. UTC | #2
Hi Ezequiel,

Thanks for your suggestion.

On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote:
> +danvet
> 
> Hi,
> 
> On Tue, 10 Aug 2021 at 23:58, 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.
> > 
> 
> I don't think it's a good idea to introduce the component API in the
> media subsystem. It doesn't seem to be maintained, IRC there's not
> even
> a maintainer for it, and it has some issues that were never
> addressed.
> 
> It would be really important to avoid it. Is it really needed in the
> first place?
> 
> Thanks,
> Ezequiel

For there are many hardware need to use, mt8192 is three and mt8195 is
five. Maybe need more to be used in the feature.

Each hardware has independent clk/power/iommu port/irq.
Use component interface in prob to get each component's information.
Just enable the hardware when need to use it, very convenient and
simple.

I found that there are many modules use component to manage hardware
information, such as iommu and drm etc.

Do you have any other suggestion for this architecture?

Thanks
Yunfei Dong
Ezequiel Garcia Aug. 19, 2021, 2:10 p.m. UTC | #3
On Thu, 19 Aug 2021 at 04:13, yunfei.dong@mediatek.com
<yunfei.dong@mediatek.com> wrote:
>
> Hi Ezequiel,
>
> Thanks for your suggestion.
>
> On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote:
> > +danvet
> >
> > Hi,
> >
> > On Tue, 10 Aug 2021 at 23:58, 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.
> > >
> >
> > I don't think it's a good idea to introduce the component API in the
> > media subsystem. It doesn't seem to be maintained, IRC there's not
> > even
> > a maintainer for it, and it has some issues that were never
> > addressed.
> >
> > It would be really important to avoid it. Is it really needed in the
> > first place?
> >
> > Thanks,
> > Ezequiel
>
> For there are many hardware need to use, mt8192 is three and mt8195 is
> five. Maybe need more to be used in the feature.
>
> Each hardware has independent clk/power/iommu port/irq.
> Use component interface in prob to get each component's information.
> Just enable the hardware when need to use it, very convenient and
> simple.
>
> I found that there are many modules use component to manage hardware
> information, such as iommu and drm etc.
>

Many drivers support multiple hardware variants, where each variant
has a different number of clocks or interrupts, see for instance
struct hantro_variant which allows to expose different codec cores,
some having both decoder/encoder, and some having just a decoder.

The component API is mostly used by DRM to aggregate independent
subdevices (called components) into an aggregated driver.

For instance, a DRM driver needs to glue together the HDMI, MIPI,
and plany controller, or any other hardware arrangement where
devices can be described independently.

The component API may look simple but has some issues, it's not easy
to debug, and can cause troubles if not used as expected [1].
It's worth making sure you actually need a framework
to glue different devices together.

> Do you have any other suggestion for this architecture?
>

Looking at the different patchsets that are posted, it's not clear
to me what exactly are the different architectures that you intend
to support, can you some documentation which clarifies that?

Thanks,
Ezequiel

[1] https://patchwork.kernel.org/project/linux-rockchip/cover/20200120170602.3832-1-ezequiel@collabora.com/
Yunfei Dong Aug. 20, 2021, 7:59 a.m. UTC | #4
Hi Ezequiel,

Thanks for your detail feedback. 

On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote:
> On Thu, 19 Aug 2021 at 04:13, yunfei.dong@mediatek.com
> <yunfei.dong@mediatek.com> wrote:
> >
> > Hi Ezequiel,
> >
> > Thanks for your suggestion.
> >
> > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote:
> > > +danvet
> > >
> > > Hi,
> > >
> > > On Tue, 10 Aug 2021 at 23:58, 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.
> > > >
> > >
> > > I don't think it's a good idea to introduce the component API in the
> > > media subsystem. It doesn't seem to be maintained, IRC there's not
> > > even
> > > a maintainer for it, and it has some issues that were never
> > > addressed.
> > >
> > > It would be really important to avoid it. Is it really needed in the
> > > first place?
> > >
> > > Thanks,
> > > Ezequiel
> >
> > For there are many hardware need to use, mt8192 is three and mt8195 is
> > five. Maybe need more to be used in the feature.
> >
> > Each hardware has independent clk/power/iommu port/irq.
> > Use component interface in prob to get each component's information.
> > Just enable the hardware when need to use it, very convenient and
> > simple.
> >
> > I found that there are many modules use component to manage hardware
> > information, such as iommu and drm etc.
> >
> 
> Many drivers support multiple hardware variants, where each variant
> has a different number of clocks or interrupts, see for instance
> struct hantro_variant which allows to expose different codec cores,
> some having both decoder/encoder, and some having just a decoder.
> 
> The component API is mostly used by DRM to aggregate independent
> subdevices (called components) into an aggregated driver.
> 
> For instance, a DRM driver needs to glue together the HDMI, MIPI,
> and plany controller, or any other hardware arrangement where
> devices can be described independently.
> 
The usage scenario is very similar with drm and iommu, So decide to use
component framework.
Decode has three/five or more hardwares, these hardware are independent.
For mt8183 just need core hardware to decode, but mt8192 has lat,soc and
core hardware to decode. When lat need to use, just enable lat hardware,
core is the same.And mt8195 will has two cores, each core can work well
independent.

For each component device just used to open their power/clk/iommu
port/irq when master need to enable it. The main logic is in master
device.

> The component API may look simple but has some issues, it's not easy
> to debug, and can cause troubles if not used as expected [1].
> It's worth making sure you actually need a framework
> to glue different devices together.
> 
Each hardware has its index, master can get hardware information
according these index, looks not complex. What do you mean about not
easy to debug?

> > Do you have any other suggestion for this architecture?
> >
> 
> Looking at the different patchsets that are posted, it's not clear
> to me what exactly are the different architectures that you intend
> to support, can you some documentation which clarifies that?
> 
Have five hardwares lat,soc,core0,core1 and main. Lat thread can use lat
soc and main, core thread can use soc,lat, core0 and core1. Core thread
can be used or not for different project. Also Need to use these
hardware dynamic at the same time. So I use component framework, just
need to know the used  hardware index according to different
project.Need not to do complex logic to manage these hardwares.


> Thanks,
> Ezequiel
> 
> [1] https://patchwork.kernel.org/project/linux-rockchip/cover/20200120170602.3832-1-ezequiel@collabora.com/


Thanks,
Yunfei Dong
Ezequiel Garcia Aug. 22, 2021, 2:32 p.m. UTC | #5
On Fri, 20 Aug 2021 at 04:59, yunfei.dong@mediatek.com
<yunfei.dong@mediatek.com> wrote:
>
> Hi Ezequiel,
>
> Thanks for your detail feedback.
>
> On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote:
> > On Thu, 19 Aug 2021 at 04:13, yunfei.dong@mediatek.com
> > <yunfei.dong@mediatek.com> wrote:
> > >
> > > Hi Ezequiel,
> > >
> > > Thanks for your suggestion.
> > >
> > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote:
> > > > +danvet
> > > >
> > > > Hi,
> > > >
> > > > On Tue, 10 Aug 2021 at 23:58, 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.
> > > > >
> > > >
> > > > I don't think it's a good idea to introduce the component API in the
> > > > media subsystem. It doesn't seem to be maintained, IRC there's not
> > > > even
> > > > a maintainer for it, and it has some issues that were never
> > > > addressed.
> > > >
> > > > It would be really important to avoid it. Is it really needed in the
> > > > first place?
> > > >
> > > > Thanks,
> > > > Ezequiel
> > >
> > > For there are many hardware need to use, mt8192 is three and mt8195 is
> > > five. Maybe need more to be used in the feature.
> > >
> > > Each hardware has independent clk/power/iommu port/irq.
> > > Use component interface in prob to get each component's information.
> > > Just enable the hardware when need to use it, very convenient and
> > > simple.
> > >
> > > I found that there are many modules use component to manage hardware
> > > information, such as iommu and drm etc.
> > >
> >
> > Many drivers support multiple hardware variants, where each variant
> > has a different number of clocks or interrupts, see for instance
> > struct hantro_variant which allows to expose different codec cores,
> > some having both decoder/encoder, and some having just a decoder.
> >
> > The component API is mostly used by DRM to aggregate independent
> > subdevices (called components) into an aggregated driver.
> >
> > For instance, a DRM driver needs to glue together the HDMI, MIPI,
> > and plany controller, or any other hardware arrangement where
> > devices can be described independently.
> >
> The usage scenario is very similar with drm and iommu, So decide to use
> component framework.
> Decode has three/five or more hardwares, these hardware are independent.
> For mt8183 just need core hardware to decode, but mt8192 has lat,soc and
> core hardware to decode. When lat need to use, just enable lat hardware,
> core is the same.And mt8195 will has two cores, each core can work well
> independent.
>
> For each component device just used to open their power/clk/iommu
> port/irq when master need to enable it. The main logic is in master
> device.
>
> > The component API may look simple but has some issues, it's not easy
> > to debug, and can cause troubles if not used as expected [1].
> > It's worth making sure you actually need a framework
> > to glue different devices together.
> >
> Each hardware has its index, master can get hardware information
> according these index, looks not complex. What do you mean about not
> easy to debug?
>
> > > Do you have any other suggestion for this architecture?
> > >
> >
> > Looking at the different patchsets that are posted, it's not clear
> > to me what exactly are the different architectures that you intend
> > to support, can you some documentation which clarifies that?
> >
> Have five hardwares lat,soc,core0,core1 and main. Lat thread can use lat
> soc and main, core thread can use soc,lat, core0 and core1. Core thread
> can be used or not for different project.

Can you explain what are these lat,soc and core threads for?

> Also Need to use these
> hardware dynamic at the same time. So I use component framework, just
> need to know the used  hardware index according to different
> project.Need not to do complex logic to manage these hardwares.
>

I am not thrilled to see the component framework introduced to the
media subsystem. Like I said, it has no clear maintainer, and it's not
easy to use.

The media subsystem has some support which AFAIK does the same thing,
see v4l2-async, which is maintained by media people.

Please push a branch based on media/master containing these changes.
I see there are other patch series for this device, but it's hard to track
which goes first, etc.

Thanks,
Ezequiel
Daniel Vetter Aug. 22, 2021, 4:50 p.m. UTC | #6
On Wed, Aug 18, 2021 at 4:12 PM Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
> +danvet
>
> Hi,
>
> On Tue, 10 Aug 2021 at 23:58, 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.
> >
>
> I don't think it's a good idea to introduce the component API in the
> media subsystem. It doesn't seem to be maintained, IRC there's not even
> a maintainer for it, and it has some issues that were never addressed.

Defacto dri-devel folks are maintainer component.c, but also I'm not
aware of anything missing there?

There has been discussions that in various drm subsystems like
drm_bridge or drm_panel a few things are missing, which prevent
drivers from moving _away_ from component.c to the more specific
solutions for panel/bridges. But nothing that's preventing them from
using component.c itself.

I'm happy to merge a MAINTAINERS patch to clarify the situation if
that's needed.
-Daniel

> It would be really important to avoid it. Is it really needed in the
> first place?
>
> Thanks,
> Ezequiel
Ezequiel Garcia Aug. 22, 2021, 5:57 p.m. UTC | #7
On Sun, 22 Aug 2021 at 13:50, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Aug 18, 2021 at 4:12 PM Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> >
> > +danvet
> >
> > Hi,
> >
> > On Tue, 10 Aug 2021 at 23:58, 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.
> > >
> >
> > I don't think it's a good idea to introduce the component API in the
> > media subsystem. It doesn't seem to be maintained, IRC there's not even
> > a maintainer for it, and it has some issues that were never addressed.
>
> Defacto dri-devel folks are maintainer component.c, but also I'm not
> aware of anything missing there?
>

A while ago, I tried to fix a crash in the Rockchip DRM driver
(I was then told there can be similar issues on the IMX driver too,
but I forgot the details of that).

I sent a patchset trying to address it and got total silence back.
Although you could argue the issue is in how drivers use the component
API, AFAICR the abuse is spreaded across a few drivers, so it felt
more reasonable to improve the component API itself, instead of changing
all the drivers.

See below:

https://patchwork.kernel.org/project/linux-rockchip/cover/20200120170602.3832-1-ezequiel@collabora.com/

> There has been discussions that in various drm subsystems like
> drm_bridge or drm_panel a few things are missing, which prevent
> drivers from moving _away_ from component.c to the more specific
> solutions for panel/bridges. But nothing that's preventing them from
> using component.c itself.
>
> I'm happy to merge a MAINTAINERS patch to clarify the situation if
> that's needed.

Indeed, that would be good.

Thanks,
Ezequiel
Yunfei Dong Aug. 24, 2021, 10:21 a.m. UTC | #8
Hi Ezequiel,

Thanks for your suggestion.

On Sun, 2021-08-22 at 11:32 -0300, Ezequiel Garcia wrote:
> On Fri, 20 Aug 2021 at 04:59, yunfei.dong@mediatek.com
> <yunfei.dong@mediatek.com> wrote:
> > 
> > Hi Ezequiel,
> > 
> > Thanks for your detail feedback.
> > 
> > On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote:
> > > On Thu, 19 Aug 2021 at 04:13, yunfei.dong@mediatek.com
> > > <yunfei.dong@mediatek.com> wrote:
> > > > 
> > > > Hi Ezequiel,
> > > > 
> > > > Thanks for your suggestion.
> > > > 
> > > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote:
> > > > > +danvet
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > On Tue, 10 Aug 2021 at 23:58, 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.
> > > > > > 
> > > > > 
> > > > > I don't think it's a good idea to introduce the component API
> > > > > in the
> > > > > media subsystem. It doesn't seem to be maintained, IRC
> > > > > there's not
> > > > > even
> > > > > a maintainer for it, and it has some issues that were never
> > > > > addressed.
> > > > > 
> > > > > It would be really important to avoid it. Is it really needed
> > > > > in the
> > > > > first place?
> > > > > 
> > > > > Thanks,
> > > > > Ezequiel
> > > > 
> > > > For there are many hardware need to use, mt8192 is three and
> > > > mt8195 is
> > > > five. Maybe need more to be used in the feature.
> > > > 
> > > > Each hardware has independent clk/power/iommu port/irq.
> > > > Use component interface in prob to get each component's
> > > > information.
> > > > Just enable the hardware when need to use it, very convenient
> > > > and
> > > > simple.
> > > > 
> > > > I found that there are many modules use component to manage
> > > > hardware
> > > > information, such as iommu and drm etc.
> > > > 
> > > 
> > > Many drivers support multiple hardware variants, where each
> > > variant
> > > has a different number of clocks or interrupts, see for instance
> > > struct hantro_variant which allows to expose different codec
> > > cores,
> > > some having both decoder/encoder, and some having just a decoder.
> > > 
> > > The component API is mostly used by DRM to aggregate independent
> > > subdevices (called components) into an aggregated driver.
> > > 
> > > For instance, a DRM driver needs to glue together the HDMI, MIPI,
> > > and plany controller, or any other hardware arrangement where
> > > devices can be described independently.
> > > 
> > 
> > The usage scenario is very similar with drm and iommu, So decide to
> > use
> > component framework.
> > Decode has three/five or more hardwares, these hardware are
> > independent.
> > For mt8183 just need core hardware to decode, but mt8192 has
> > lat,soc and
> > core hardware to decode. When lat need to use, just enable lat
> > hardware,
> > core is the same.And mt8195 will has two cores, each core can work
> > well
> > independent.
> > 
> > For each component device just used to open their power/clk/iommu
> > port/irq when master need to enable it. The main logic is in master
> > device.
> > 
> > > The component API may look simple but has some issues, it's not
> > > easy
> > > to debug, and can cause troubles if not used as expected [1].
> > > It's worth making sure you actually need a framework
> > > to glue different devices together.
> > > 
> > 
> > Each hardware has its index, master can get hardware information
> > according these index, looks not complex. What do you mean about
> > not
> > easy to debug?
> > 
> > > > Do you have any other suggestion for this architecture?
> > > > 
> > > 
> > > Looking at the different patchsets that are posted, it's not
> > > clear
> > > to me what exactly are the different architectures that you
> > > intend
> > > to support, can you some documentation which clarifies that?
> > > 
> > 
> > Have five hardwares lat,soc,core0,core1 and main. Lat thread can
> > use lat
> > soc and main, core thread can use soc,lat, core0 and core1. Core
> > thread
> > can be used or not for different project.
> 
> Can you explain what are these lat,soc and core threads for?
> 
You can regards lat,soc and core as hardware, each hardware can work
independent. Lat and core threads used to control hardware to decode.
> > Also Need to use these
> > hardware dynamic at the same time. So I use component framework,
> > just
> > need to know the used  hardware index according to different
> > project.Need not to do complex logic to manage these hardwares.
> > 
> 
> I am not thrilled to see the component framework introduced to the
> media subsystem. Like I said, it has no clear maintainer, and it's
> not
> easy to use.
> 
How do you think about Deniel Vetter's mail ? It looks that there are
maintainer for it.
> The media subsystem has some support which AFAIK does the same thing,
> see v4l2-async, which is maintained by media people.
> 
If component can be used, I prefer to use it. At the other hand, I will
try to use these method as compared.
> Please push a branch based on media/master containing these changes.
> I see there are other patch series for this device, but it's hard to
> track
> which goes first, etc.
> 
I need time to push a branch, or you can get the latest kernel and git
am these patches, maybe much quicker.
> Thanks,
> Ezequiel

Thanks,
Yunfei Dong
Yunfei Dong Aug. 25, 2021, 5:48 a.m. UTC | #9
Hi Ezequiel,

You can get the dtsi information from patch 13, it is decoder yaml file
about component architecture:

[PATCH v4, 13/15] dt-bindings: media: mtk-vcodec: Adds decoder dt-
bindings for mt8192

Thanks
Yunfei Dong

On Tue, 2021-08-24 at 18:21 +0800, yunfei.dong@mediatek.com wrote:
> Hi Ezequiel,
> 
> Thanks for your suggestion.
> 
> On Sun, 2021-08-22 at 11:32 -0300, Ezequiel Garcia wrote:
> > On Fri, 20 Aug 2021 at 04:59, yunfei.dong@mediatek.com
> > <yunfei.dong@mediatek.com> wrote:
> > > 
> > > Hi Ezequiel,
> > > 
> > > Thanks for your detail feedback.
> > > 
> > > On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote:
> > > > On Thu, 19 Aug 2021 at 04:13, yunfei.dong@mediatek.com
> > > > <yunfei.dong@mediatek.com> wrote:
> > > > > 
> > > > > Hi Ezequiel,
> > > > > 
> > > > > Thanks for your suggestion.
> > > > > 
> > > > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote:
> > > > > > +danvet
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Tue, 10 Aug 2021 at 23:58, 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.
> > > > > > > 
> > > > > > 
> > > > > > I don't think it's a good idea to introduce the component
> > > > > > API
> > > > > > in the
> > > > > > media subsystem. It doesn't seem to be maintained, IRC
> > > > > > there's not
> > > > > > even
> > > > > > a maintainer for it, and it has some issues that were never
> > > > > > addressed.
> > > > > > 
> > > > > > It would be really important to avoid it. Is it really
> > > > > > needed
> > > > > > in the
> > > > > > first place?
> > > > > > 
> > > > > > Thanks,
> > > > > > Ezequiel
> > > > > 
> > > > > For there are many hardware need to use, mt8192 is three and
> > > > > mt8195 is
> > > > > five. Maybe need more to be used in the feature.
> > > > > 
> > > > > Each hardware has independent clk/power/iommu port/irq.
> > > > > Use component interface in prob to get each component's
> > > > > information.
> > > > > Just enable the hardware when need to use it, very convenient
> > > > > and
> > > > > simple.
> > > > > 
> > > > > I found that there are many modules use component to manage
> > > > > hardware
> > > > > information, such as iommu and drm etc.
> > > > > 
> > > > 
> > > > Many drivers support multiple hardware variants, where each
> > > > variant
> > > > has a different number of clocks or interrupts, see for
> > > > instance
> > > > struct hantro_variant which allows to expose different codec
> > > > cores,
> > > > some having both decoder/encoder, and some having just a
> > > > decoder.
> > > > 
> > > > The component API is mostly used by DRM to aggregate
> > > > independent
> > > > subdevices (called components) into an aggregated driver.
> > > > 
> > > > For instance, a DRM driver needs to glue together the HDMI,
> > > > MIPI,
> > > > and plany controller, or any other hardware arrangement where
> > > > devices can be described independently.
> > > > 
> > > 
> > > The usage scenario is very similar with drm and iommu, So decide
> > > to
> > > use
> > > component framework.
> > > Decode has three/five or more hardwares, these hardware are
> > > independent.
> > > For mt8183 just need core hardware to decode, but mt8192 has
> > > lat,soc and
> > > core hardware to decode. When lat need to use, just enable lat
> > > hardware,
> > > core is the same.And mt8195 will has two cores, each core can
> > > work
> > > well
> > > independent.
> > > 
> > > For each component device just used to open their power/clk/iommu
> > > port/irq when master need to enable it. The main logic is in
> > > master
> > > device.
> > > 
> > > > The component API may look simple but has some issues, it's not
> > > > easy
> > > > to debug, and can cause troubles if not used as expected [1].
> > > > It's worth making sure you actually need a framework
> > > > to glue different devices together.
> > > > 
> > > 
> > > Each hardware has its index, master can get hardware information
> > > according these index, looks not complex. What do you mean about
> > > not
> > > easy to debug?
> > > 
> > > > > Do you have any other suggestion for this architecture?
> > > > > 
> > > > 
> > > > Looking at the different patchsets that are posted, it's not
> > > > clear
> > > > to me what exactly are the different architectures that you
> > > > intend
> > > > to support, can you some documentation which clarifies that?
> > > > 
> > > 
> > > Have five hardwares lat,soc,core0,core1 and main. Lat thread can
> > > use lat
> > > soc and main, core thread can use soc,lat, core0 and core1. Core
> > > thread
> > > can be used or not for different project.
> > 
> > Can you explain what are these lat,soc and core threads for?
> > 
> 
> You can regards lat,soc and core as hardware, each hardware can work
> independent. Lat and core threads used to control hardware to decode.
> > > Also Need to use these
> > > hardware dynamic at the same time. So I use component framework,
> > > just
> > > need to know the used  hardware index according to different
> > > project.Need not to do complex logic to manage these hardwares.
> > > 
> > 
> > I am not thrilled to see the component framework introduced to the
> > media subsystem. Like I said, it has no clear maintainer, and it's
> > not
> > easy to use.
> > 
> 
> How do you think about Deniel Vetter's mail ? It looks that there are
> maintainer for it.
> > The media subsystem has some support which AFAIK does the same
> > thing,
> > see v4l2-async, which is maintained by media people.
> > 
> 
> If component can be used, I prefer to use it. At the other hand, I
> will
> try to use these method as compared.
> > Please push a branch based on media/master containing these
> > changes.
> > I see there are other patch series for this device, but it's hard
> > to
> > track
> > which goes first, etc.
> > 
> 
> I need time to push a branch, or you can get the latest kernel and
> git
> am these patches, maybe much quicker.
> > Thanks,
> > Ezequiel
> 
> Thanks,
> Yunfei Dong
Daniel Vetter Aug. 26, 2021, 9:14 a.m. UTC | #10
On Sun, Aug 22, 2021 at 02:57:15PM -0300, Ezequiel Garcia wrote:
> On Sun, 22 Aug 2021 at 13:50, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Aug 18, 2021 at 4:12 PM Ezequiel Garcia
> > <ezequiel@vanguardiasur.com.ar> wrote:
> > >
> > > +danvet
> > >
> > > Hi,
> > >
> > > On Tue, 10 Aug 2021 at 23:58, 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.
> > > >
> > >
> > > I don't think it's a good idea to introduce the component API in the
> > > media subsystem. It doesn't seem to be maintained, IRC there's not even
> > > a maintainer for it, and it has some issues that were never addressed.
> >
> > Defacto dri-devel folks are maintainer component.c, but also I'm not
> > aware of anything missing there?
> >
> 
> A while ago, I tried to fix a crash in the Rockchip DRM driver
> (I was then told there can be similar issues on the IMX driver too,
> but I forgot the details of that).
> 
> I sent a patchset trying to address it and got total silence back.
> Although you could argue the issue is in how drivers use the component
> API, AFAICR the abuse is spreaded across a few drivers, so it felt
> more reasonable to improve the component API itself, instead of changing
> all the drivers.
> 
> See below:
> 
> https://patchwork.kernel.org/project/linux-rockchip/cover/20200120170602.3832-1-ezequiel@collabora.com/

Patches get lost on the mailing list, and rockchip is one of the lesser
maintained drivers. You need to ping this stuff.

For bridge/panel I still think we should work towards removing component.c
use from them.

> > There has been discussions that in various drm subsystems like
> > drm_bridge or drm_panel a few things are missing, which prevent
> > drivers from moving _away_ from component.c to the more specific
> > solutions for panel/bridges. But nothing that's preventing them from
> > using component.c itself.
> >
> > I'm happy to merge a MAINTAINERS patch to clarify the situation if
> > that's needed.
> 
> Indeed, that would be good.

Ok I'm going to type something.