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.
> + if (err < 0)
> + return err;
> +
> + if (vic->can_use_context)
> + return 0x30;
> + else
> + return -ENOTSUPP;
If (!vic->can_use_context)
return -ENOTSUPP;
return 0x30;
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.
>
>> + if (err < 0)
>> + return err;
>> +
>> + if (vic->can_use_context)
>> + return 0x30;
>> + else
>> + return -ENOTSUPP;
>
> If (!vic->can_use_context)
> return -ENOTSUPP;
>
> return 0x30;
and s/ENOTSUPP/EOPNOTSUPP/
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.
>>
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + if (vic->can_use_context)
>>> + return 0x30;
>>> + else
>>> + return -ENOTSUPP;
>>
>> If (!vic->can_use_context)
>> return -ENOTSUPP;
>>
>> return 0x30;
>
> and s/ENOTSUPP/EOPNOTSUPP/
Ok.
Mikko
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.
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.
Mikko
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.