[PATCH v2 0/5] Enable decoder for mt8183

[PATCH v2 0/5] Enable decoder for mt8183

[Nícolas F. R. A. Prado]

This series enables the hardware decoder present on mt8183. At first
glance, the only missing piece is the devicetree node for it, however,
simply adding it as is would cause an address collision between the
first register iospace and the clock-controller node, so a rework of the
dt-binding and driver, as well as addition of a clock, were needed
first.

Tested that H264 decoding works with the hardware decoder on
mt8183-kukui-jacuzzi-juniper-sku16, giving a fluster score of 98/135 on
the JVT-AVC_V1 test suite. And ensured other SoCs (MT8192 and MT8195)
still work as usual.

Changes in v2:
- Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
  clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
  Remove VDEC_SYS for mt8183)
- Further constrained properties in dt-binding
- Added CLK_IGNORE_UNUSED flag to active clock
- Reformatted reg-names in DT node

Nícolas F. R. A. Prado (4):
  media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks
  media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183
  media: mediatek: vcodec: Read HW active status from clock
  clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

Yunfei Dong (1):
  arm64: dts: mediatek: mt8183: Add decoder

 .../media/mediatek,vcodec-decoder.yaml        | 65 +++++++++++++++----
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 +++++++++
 drivers/clk/mediatek/clk-mt8183-vdec.c        |  5 ++
 .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 59 +++++++++++++----
 .../mediatek/vcodec/mtk_vcodec_dec_hw.c       | 20 ++++--
 .../mediatek/vcodec/mtk_vcodec_dec_pm.c       | 12 +++-
 .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
 include/dt-bindings/clock/mt8183-clk.h        |  3 +-
 8 files changed, 165 insertions(+), 30 deletions(-)

-- 
2.41.0

[PATCH v2 4/5] clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

[Nícolas F. R. A. Prado]

Add the CLK_VDEC_ACTIVE clock to the vdec clock driver. This clock is
enabled by the VPU once it starts decoding.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

Changes in v2:
- Added CLK_IGNORE_UNUSED flag

 drivers/clk/mediatek/clk-mt8183-vdec.c | 5 +++++
 include/dt-bindings/clock/mt8183-clk.h | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mediatek/clk-mt8183-vdec.c b/drivers/clk/mediatek/clk-mt8183-vdec.c
index 513b7956cbea..03c4f1acfdb8 100644
--- a/drivers/clk/mediatek/clk-mt8183-vdec.c
+++ b/drivers/clk/mediatek/clk-mt8183-vdec.c
@@ -27,6 +27,10 @@ static const struct mtk_gate_regs vdec1_cg_regs = {
 	GATE_MTK(_id, _name, _parent, &vdec0_cg_regs, _shift,	\
 		&mtk_clk_gate_ops_setclr_inv)
 
+#define GATE_VDEC0(_id, _name, _parent, _shift)		\
+	GATE_MTK_FLAGS(_id, _name, _parent, &vdec0_cg_regs, _shift,	\
+		&mtk_clk_gate_ops_setclr, CLK_IGNORE_UNUSED)
+
 #define GATE_VDEC1_I(_id, _name, _parent, _shift)		\
 	GATE_MTK(_id, _name, _parent, &vdec1_cg_regs, _shift,	\
 		&mtk_clk_gate_ops_setclr_inv)
@@ -34,6 +38,7 @@ static const struct mtk_gate_regs vdec1_cg_regs = {
 static const struct mtk_gate vdec_clks[] = {
 	/* VDEC0 */
 	GATE_VDEC0_I(CLK_VDEC_VDEC, "vdec_vdec", "mm_sel", 0),
+	GATE_VDEC0(CLK_VDEC_ACTIVE, "vdec_active", "mm_sel", 4),
 	/* VDEC1 */
 	GATE_VDEC1_I(CLK_VDEC_LARB1, "vdec_larb1", "mm_sel", 0),
 };
diff --git a/include/dt-bindings/clock/mt8183-clk.h b/include/dt-bindings/clock/mt8183-clk.h
index a7b470b0ec8a..32dd7d91dbe2 100644
--- a/include/dt-bindings/clock/mt8183-clk.h
+++ b/include/dt-bindings/clock/mt8183-clk.h
@@ -357,7 +357,8 @@
 /* VDEC_GCON */
 #define CLK_VDEC_VDEC			0
 #define CLK_VDEC_LARB1			1
-#define CLK_VDEC_NR_CLK			2
+#define CLK_VDEC_ACTIVE			2
+#define CLK_VDEC_NR_CLK			3
 
 /* VENC_GCON */
 #define CLK_VENC_LARB			0
-- 
2.41.0

Re: [PATCH v2 4/5] clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

[AngeloGioacchino Del Regno]

Il 07/06/23 22:53, Nícolas F. R. A. Prado ha scritto:
> Add the CLK_VDEC_ACTIVE clock to the vdec clock driver. This clock is
> enabled by the VPU once it starts decoding.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 4/5] clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

[Chen-Yu Tsai]

On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Add the CLK_VDEC_ACTIVE clock to the vdec clock driver. This clock is
> enabled by the VPU once it starts decoding.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> ---
>
> Changes in v2:
> - Added CLK_IGNORE_UNUSED flag
>
>  drivers/clk/mediatek/clk-mt8183-vdec.c | 5 +++++
>  include/dt-bindings/clock/mt8183-clk.h | 3 ++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8183-vdec.c b/drivers/clk/mediatek/clk-mt8183-vdec.c
> index 513b7956cbea..03c4f1acfdb8 100644
> --- a/drivers/clk/mediatek/clk-mt8183-vdec.c
> +++ b/drivers/clk/mediatek/clk-mt8183-vdec.c
> @@ -27,6 +27,10 @@ static const struct mtk_gate_regs vdec1_cg_regs = {
>         GATE_MTK(_id, _name, _parent, &vdec0_cg_regs, _shift,   \
>                 &mtk_clk_gate_ops_setclr_inv)
>
> +#define GATE_VDEC0(_id, _name, _parent, _shift)                \
> +       GATE_MTK_FLAGS(_id, _name, _parent, &vdec0_cg_regs, _shift,     \
> +               &mtk_clk_gate_ops_setclr, CLK_IGNORE_UNUSED)

I think what you want is a read-only gate clock only used for reading back
the status. The ops would only have .is_enabled.

ChenYu

Re: [PATCH v2 4/5] clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

[AngeloGioacchino Del Regno]

Il 08/06/23 09:43, Chen-Yu Tsai ha scritto:
> On Thu, Jun 8, 2023 at 4:57 AM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
>>
>> Add the CLK_VDEC_ACTIVE clock to the vdec clock driver. This clock is
>> enabled by the VPU once it starts decoding.
>>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>
>> ---
>>
>> Changes in v2:
>> - Added CLK_IGNORE_UNUSED flag
>>
>>   drivers/clk/mediatek/clk-mt8183-vdec.c | 5 +++++
>>   include/dt-bindings/clock/mt8183-clk.h | 3 ++-
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8183-vdec.c b/drivers/clk/mediatek/clk-mt8183-vdec.c
>> index 513b7956cbea..03c4f1acfdb8 100644
>> --- a/drivers/clk/mediatek/clk-mt8183-vdec.c
>> +++ b/drivers/clk/mediatek/clk-mt8183-vdec.c
>> @@ -27,6 +27,10 @@ static const struct mtk_gate_regs vdec1_cg_regs = {
>>          GATE_MTK(_id, _name, _parent, &vdec0_cg_regs, _shift,   \
>>                  &mtk_clk_gate_ops_setclr_inv)
>>
>> +#define GATE_VDEC0(_id, _name, _parent, _shift)                \
>> +       GATE_MTK_FLAGS(_id, _name, _parent, &vdec0_cg_regs, _shift,     \
>> +               &mtk_clk_gate_ops_setclr, CLK_IGNORE_UNUSED)
> 
> I think what you want is a read-only gate clock only used for reading back
> the status. The ops would only have .is_enabled.

Technically, you're right... but I would delay the introduction of a RO GATE_MTK
clock for later, as it's not worth adding that for just one clock driver usage.

We're checking if the same can be applied to other SoCs as well - if it can,
then it would make sense to do that (small, yes), work... the point here is to
enable MT8183 decoders ASAP to enable decoder tests in KernelCI, along with
all of the other MediaTek Chromebooks.

Though, if you think that it is a good idea to add a RO gate right now, I don't
have any strong opinions against that.

Cheers,
Angelo

Re: [PATCH v2 0/5] Enable decoder for mt8183

[Hans Verkuil]

Hi Nicolas,

On 07/06/2023 22:53, Nícolas F. R. A. Prado wrote:
> 
> This series enables the hardware decoder present on mt8183. At first
> glance, the only missing piece is the devicetree node for it, however,
> simply adding it as is would cause an address collision between the
> first register iospace and the clock-controller node, so a rework of the
> dt-binding and driver, as well as addition of a clock, were needed
> first.
> 
> Tested that H264 decoding works with the hardware decoder on
> mt8183-kukui-jacuzzi-juniper-sku16, giving a fluster score of 98/135 on
> the JVT-AVC_V1 test suite. And ensured other SoCs (MT8192 and MT8195)
> still work as usual.
> 
> Changes in v2:
> - Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
>   clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
>   Remove VDEC_SYS for mt8183)
> - Further constrained properties in dt-binding
> - Added CLK_IGNORE_UNUSED flag to active clock
> - Reformatted reg-names in DT node
> 
> Nícolas F. R. A. Prado (4):
>   media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks
>   media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183
>   media: mediatek: vcodec: Read HW active status from clock
>   clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec

Is the clk patch independent from the others? It's not clear to me.

If the clk patch has to go in together with the media patches, then
please let me know and post a v3 where the clk patch is also CC-ed to
the linux-media mailinglist to ensure it ends up in our patchwork system.

And in that case I need a Acked-by from the clk maintainer as well.

If it is independent, then there is no need for a v3 (at least, not
for this).

Regards,

	Hans

> 
> Yunfei Dong (1):
>   arm64: dts: mediatek: mt8183: Add decoder
> 
>  .../media/mediatek,vcodec-decoder.yaml        | 65 +++++++++++++++----
>  arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 +++++++++
>  drivers/clk/mediatek/clk-mt8183-vdec.c        |  5 ++
>  .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 59 +++++++++++++----
>  .../mediatek/vcodec/mtk_vcodec_dec_hw.c       | 20 ++++--
>  .../mediatek/vcodec/mtk_vcodec_dec_pm.c       | 12 +++-
>  .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
>  include/dt-bindings/clock/mt8183-clk.h        |  3 +-
>  8 files changed, 165 insertions(+), 30 deletions(-)
> 

Re: [PATCH v2 0/5] Enable decoder for mt8183

[AngeloGioacchino Del Regno]

Il 12/06/23 09:02, Hans Verkuil ha scritto:
> Hi Nicolas,
> 
> On 07/06/2023 22:53, Nícolas F. R. A. Prado wrote:
>>
>> This series enables the hardware decoder present on mt8183. At first
>> glance, the only missing piece is the devicetree node for it, however,
>> simply adding it as is would cause an address collision between the
>> first register iospace and the clock-controller node, so a rework of the
>> dt-binding and driver, as well as addition of a clock, were needed
>> first.
>>
>> Tested that H264 decoding works with the hardware decoder on
>> mt8183-kukui-jacuzzi-juniper-sku16, giving a fluster score of 98/135 on
>> the JVT-AVC_V1 test suite. And ensured other SoCs (MT8192 and MT8195)
>> still work as usual.
>>
>> Changes in v2:
>> - Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
>>    clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
>>    Remove VDEC_SYS for mt8183)
>> - Further constrained properties in dt-binding
>> - Added CLK_IGNORE_UNUSED flag to active clock
>> - Reformatted reg-names in DT node
>>
>> Nícolas F. R. A. Prado (4):
>>    media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks
>>    media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183
>>    media: mediatek: vcodec: Read HW active status from clock
>>    clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec
> 
> Is the clk patch independent from the others? It's not clear to me.
> 
> If the clk patch has to go in together with the media patches, then
> please let me know and post a v3 where the clk patch is also CC-ed to
> the linux-media mailinglist to ensure it ends up in our patchwork system.
> 
> And in that case I need a Acked-by from the clk maintainer as well.
> 
> If it is independent, then there is no need for a v3 (at least, not
> for this).
> 

The clock patch is not independent, as in the devicetree changes will not
work without the addition of that clock (and of course even fail building),
so that series needs a v3.

Nícolas, please go on and send a v3 as requested.

Cheers,
Angelo

> Regards,
> 
> 	Hans
> 
>>
>> Yunfei Dong (1):
>>    arm64: dts: mediatek: mt8183: Add decoder
>>
>>   .../media/mediatek,vcodec-decoder.yaml        | 65 +++++++++++++++----
>>   arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 30 +++++++++
>>   drivers/clk/mediatek/clk-mt8183-vdec.c        |  5 ++
>>   .../mediatek/vcodec/mtk_vcodec_dec_drv.c      | 59 +++++++++++++----
>>   .../mediatek/vcodec/mtk_vcodec_dec_hw.c       | 20 ++++--
>>   .../mediatek/vcodec/mtk_vcodec_dec_pm.c       | 12 +++-
>>   .../platform/mediatek/vcodec/mtk_vcodec_drv.h |  1 +
>>   include/dt-bindings/clock/mt8183-clk.h        |  3 +-
>>   8 files changed, 165 insertions(+), 30 deletions(-)
>>
> 
> 

Re: [PATCH v2 0/5] Enable decoder for mt8183

[Nícolas F. R. A. Prado]

On Mon, Jun 12, 2023 at 11:53:51AM +0200, AngeloGioacchino Del Regno wrote:
> Il 12/06/23 09:02, Hans Verkuil ha scritto:
> > Hi Nicolas,
> > 
> > On 07/06/2023 22:53, Nícolas F. R. A. Prado wrote:
> > > 
> > > This series enables the hardware decoder present on mt8183. At first
> > > glance, the only missing piece is the devicetree node for it, however,
> > > simply adding it as is would cause an address collision between the
> > > first register iospace and the clock-controller node, so a rework of the
> > > dt-binding and driver, as well as addition of a clock, were needed
> > > first.
> > > 
> > > Tested that H264 decoding works with the hardware decoder on
> > > mt8183-kukui-jacuzzi-juniper-sku16, giving a fluster score of 98/135 on
> > > the JVT-AVC_V1 test suite. And ensured other SoCs (MT8192 and MT8195)
> > > still work as usual.
> > > 
> > > Changes in v2:
> > > - Merged commit 1 (media: dt-bindings: mediatek,vcodec: Allow single
> > >    clock for mt8183) into commit 3 (media: dt-bindings: mediatek,vcodec:
> > >    Remove VDEC_SYS for mt8183)
> > > - Further constrained properties in dt-binding
> > > - Added CLK_IGNORE_UNUSED flag to active clock
> > > - Reformatted reg-names in DT node
> > > 
> > > Nícolas F. R. A. Prado (4):
> > >    media: dt-bindings: mediatek,vcodec: Don't require assigned-clocks
> > >    media: dt-bindings: mediatek,vcodec: Remove VDEC_SYS for mt8183
> > >    media: mediatek: vcodec: Read HW active status from clock
> > >    clk: mediatek: mt8183: Add CLK_VDEC_ACTIVE to vdec
> > 
> > Is the clk patch independent from the others? It's not clear to me.
> > 
> > If the clk patch has to go in together with the media patches, then
> > please let me know and post a v3 where the clk patch is also CC-ed to
> > the linux-media mailinglist to ensure it ends up in our patchwork system.
> > 
> > And in that case I need a Acked-by from the clk maintainer as well.
> > 
> > If it is independent, then there is no need for a v3 (at least, not
> > for this).
> > 
> 
> The clock patch is not independent, as in the devicetree changes will not
> work without the addition of that clock (and of course even fail building),

Yes, but that means that the devicetree patch is dependent of the clock patch,
but the clock patch is independent from the media patches, and can therefore be
applied through the clock tree as usual.

So, the media patches (first three) can be merged through the media tree, the
clock patch (patch 4) through the clock tree independently, and the only
requirement is that the DT patch (last one) is only applied by Matthias after
the clock patch is present, to avoid build problems, and also after the media
patches are present, to avoid dt-binding errors.

Thanks,
Nícolas