Re: [PATCH v3 8/9] drm/tegra: vic: Implement get_streamid_offset

[Mikko Perttunen]

On 2/22/22 12:46, Dmitry Osipenko wrote:
> 22.02.2022 11:27, Mikko Perttunen пишет:
> > On 2/21/22 22:10, Dmitry Osipenko wrote:
> > > 21.02.2022 14:44, Mikko Perttunen пишет:
> > > > On 2/19/22 20:54, Dmitry Osipenko wrote:
> > > > > 19.02.2022 21:49, Dmitry Osipenko пишет:
> > > > > > 18.02.2022 14:39, Mikko Perttunen пишет:
> > > > > > > +static int vic_get_streamid_offset(struct tegra_drm_client *client)
> > > > > > > +{
> > > > > > > +    struct vic *vic = to_vic(client);
> > > > > > > +    int err;
> > > > > > > +
> > > > > > > +    err = vic_load_firmware(vic);
> > > > > >
> > > > > > You can't invoke vic_load_firmware() while RPM is suspended. Either
> > > > > > replace this with RPM get/put or do something else.
> > > >
> > > > Why not, I'm not seeing any HW accesses in vic_load_firmware? Although
> > > > it looks like it might race with the vic_load_firmware call in
> > > > vic_runtime_resume which probably needs to be fixed.
> > >
> > > It was not clear from the function's name that h/w is untouched, I read
> > > "load" as "upload" and then looked at vic_runtime_resume(). I'd rename
> > > vic_load_firmware() to vic_prepare_firmware_image().
> > >
> > > And yes, technically lock is needed.
> >
> > Yep, I'll consider renaming it.
>
> Looking at this all again, I'd suggest to change:
>
> int get_streamid_offset(client)
>
> to:
>
> int get_streamid_offset(client, *offset)
>
> and bail out if get_streamid_offset() returns error. It's never okay to
> ignore errors.

Sure, seems reasonable. We'll still need some error code to indicate 
that context isolation isn't available for the engine and continue on in 
that case but that's better than just ignoring all of them.

Mikko