Re: [PATCH v4 79/79] media: hantro: do a PM resume earlier

[Mauro Carvalho Chehab]

Em Wed, 28 Apr 2021 14:17:50 -0300
Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> escreveu:

> Hi Mauro,
> 
> Thanks a lot for taking care of this.
> 
> On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote:
> > The device_run() first enables the clock and then
> > tries to resume PM runtime, checking for errors.
> > 
> > Well, if for some reason the pm_runtime can not resume,
> > it would be better to detect it beforehand.
> > 
> > So, change the order inside device_run().
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>  
> 
> Clocks could be behind power-domains, IIRC, so this change
> is fixing that.
> 
> However, this has ever been a problem for this driver,
> so I don't think it makes sense to bother with Fixes tag.

I would prefer to move this patch to the first part of this
series, together with other fixes, rebasing it to apply cleanly
before the pm_runtime_resume_and_get() patch, with:

    Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")

This way, people that could be interested on backporting it will be
capable to apply it as is to stable Kernel releases that came
with this driver.

> 
> Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> 
> Thanks,
> Ezequiel
> 
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 25fa36e7e773..67de6b15236d 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -160,14 +160,14 @@ static void device_run(void *priv)
> >         src = hantro_get_src_buf(ctx);
> >         dst = hantro_get_dst_buf(ctx);
> >  
> > -       ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> > -       if (ret)
> > -               goto err_cancel_job;
> > -
> >         ret = pm_runtime_resume_and_get(ctx->dev->dev);
> >         if (ret < 0)
> >                 goto err_cancel_job;
> >  
> > +       ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> > +       if (ret)
> > +               goto err_cancel_job;
> > +
> >         v4l2_m2m_buf_copy_metadata(src, dst, true);
> >  
> >         ctx->codec_ops->run(ctx);  
> 
> 



Thanks,
Mauro