Re: [PATCH v5 13/16] media: mtk-vcodec: Get rid of mtk_smi_larb_get/put

Re: [PATCH v5 13/16] media: mtk-vcodec: Get rid of mtk_smi_larb_get/put

[Yong Wu]

On Thu, 2021-06-10 at 09:53 +0200, Matthias Brugger wrote:
> Hi Yong,
> 
> On 12/05/2021 14:29, Yong Wu wrote:
> > On Wed, 2021-05-12 at 17:20 +0800, Hsin-Yi Wang wrote:
> >> On Sat, Apr 10, 2021 at 5:14 PM Yong Wu <yong.wu@mediatek.com> wrote:
> >>>
> >>> MediaTek IOMMU has already added the device_link between the consumer
> >>> and smi-larb device. If the vcodec device call the pm_runtime_get_sync,
> >>> the smi-larb's pm_runtime_get_sync also be called automatically.
> >>>
> >>> CC: Tiffany Lin <tiffany.lin@mediatek.com>
> >>> CC: Irui Wang <irui.wang@mediatek.com>
> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >>> Reviewed-by: Evan Green <evgreen@chromium.org>
> >>> Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
> >>> ---
> >>>  .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 ++-------------
> >>>  .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
> >>>  .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
> >>>  .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 46 ++-----------------
> >>>  4 files changed, 10 insertions(+), 77 deletions(-)
> > 
> > [...]
> > 
> >>> @@ -108,13 +80,6 @@ void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm)
> >>>                 }
> >>>         }
> >>>
> >>> -       ret = mtk_smi_larb_get(pm->larbvenc);
> >>> -       if (ret) {
> >>> -               mtk_v4l2_err("mtk_smi_larb_get larb3 fail %d", ret);
> >>> -               goto clkerr;
> >>> -       }
> >>> -       return;
> >>
> >> You can't delete the return; here, otherwise vcodec_clk will be turned
> >> off immediately after they are turned on.
> > 
> > Thanks very much for your review.
> > 
> > Sorry for this. You are quite right.
> > 
> > I checked this, it was introduced in v4 when I rebase the code. I will
> > fix it in next time.
> > 
> 
> Please also make sure that you add all maintainers. I realized that at least for
> the media/platform drivers we miss the maintainer and the corresponding mailing
> list.
> This is especially important in this series, as it spans several subsystems.

Thanks for hint. I only added the file maintainer here. I will add
linux-media in next version.

By the way, this patchset cross several trees, then which tree should it
go through. Do you have some suggestion?

> 
> Thanks a lot,
> Matthias
> 
> >>
> >>> -
> >>>  clkerr:
> >>>         for (i -= 1; i >= 0; i--)
> >>>                 clk_disable_unprepare(enc_clk->clk_info[i].vcodec_clk);
> >>> @@ -125,7 +90,6 @@ void mtk_vcodec_enc_clock_off(struct mtk_vcodec_pm *pm)
> >>>         struct mtk_vcodec_clk *enc_clk = &pm->venc_clk;
> >>>         int i = 0;
> >>>
> >>> -       mtk_smi_larb_put(pm->larbvenc);
> >>>         for (i = enc_clk->clk_num - 1; i >= 0; i--)
> >>>                 clk_disable_unprepare(enc_clk->clk_info[i].vcodec_clk);
> >>>  }
> >>> --
> >>> 2.18.0
> >>>
> > 

Re: [PATCH v5 13/16] media: mtk-vcodec: Get rid of mtk_smi_larb_get/put

[Matthias Brugger]

On 10/06/2021 14:02, Yong Wu wrote:
> On Thu, 2021-06-10 at 09:53 +0200, Matthias Brugger wrote:
>> Hi Yong,
>>
>> On 12/05/2021 14:29, Yong Wu wrote:
>>> On Wed, 2021-05-12 at 17:20 +0800, Hsin-Yi Wang wrote:
>>>> On Sat, Apr 10, 2021 at 5:14 PM Yong Wu <yong.wu@mediatek.com> wrote:
>>>>>
>>>>> MediaTek IOMMU has already added the device_link between the consumer
>>>>> and smi-larb device. If the vcodec device call the pm_runtime_get_sync,
>>>>> the smi-larb's pm_runtime_get_sync also be called automatically.
>>>>>
>>>>> CC: Tiffany Lin <tiffany.lin@mediatek.com>
>>>>> CC: Irui Wang <irui.wang@mediatek.com>
>>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>>>> Reviewed-by: Evan Green <evgreen@chromium.org>
>>>>> Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
>>>>> ---
>>>>>  .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 ++-------------
>>>>>  .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
>>>>>  .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
>>>>>  .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 46 ++-----------------
>>>>>  4 files changed, 10 insertions(+), 77 deletions(-)
>>>
>>> [...]
>>>
>>>>> @@ -108,13 +80,6 @@ void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm)
>>>>>                 }
>>>>>         }
>>>>>
>>>>> -       ret = mtk_smi_larb_get(pm->larbvenc);
>>>>> -       if (ret) {
>>>>> -               mtk_v4l2_err("mtk_smi_larb_get larb3 fail %d", ret);
>>>>> -               goto clkerr;
>>>>> -       }
>>>>> -       return;
>>>>
>>>> You can't delete the return; here, otherwise vcodec_clk will be turned
>>>> off immediately after they are turned on.
>>>
>>> Thanks very much for your review.
>>>
>>> Sorry for this. You are quite right.
>>>
>>> I checked this, it was introduced in v4 when I rebase the code. I will
>>> fix it in next time.
>>>
>>
>> Please also make sure that you add all maintainers. I realized that at least for
>> the media/platform drivers we miss the maintainer and the corresponding mailing
>> list.
>> This is especially important in this series, as it spans several subsystems.
> 
> Thanks for hint. I only added the file maintainer here. I will add
> linux-media in next version.
> 
> By the way, this patchset cross several trees, then which tree should it
> go through. Do you have some suggestion?
> 

That's a good question. I think the media tree would be a good candidate, as it
has the biggest bunch of patches. But that would mean that Joerg is fine that.
The DTS part could still go through my tree.

Regards,
Matthias

Re: [PATCH v5 13/16] media: mtk-vcodec: Get rid of mtk_smi_larb_get/put

[Joerg Roedel]

On Fri, Jun 11, 2021 at 12:07:24PM +0200, Matthias Brugger wrote:
> That's a good question. I think the media tree would be a good
> candidate, as it has the biggest bunch of patches. But that would mean
> that Joerg is fine that.  The DTS part could still go through my tree.

IOMMU changes are only a minor part of this, so it should not go through
the IOMMU tree. When Matthias has reviewed the IOMMU changes, feel free
to add my

	Acked-by: Joerg Roedel <jroedel@suse.de>

to them.

Regards,

	Joerg