2023-05-04 08:04:08

by Konrad Dybcio

[permalink] [raw]
Subject: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()

This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.

Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
Reviewed-by: Bryan O'Donoghue <[email protected]>
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/vdec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 51a53bf82bd3..33e3f7208b1a 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
struct hfi_video_work_route wr;

- if (!IS_V6(inst->core))
+ if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))
return 0;

wr.video_work_route = inst->core->res->num_vpp_pipes;

--
2.40.1


2023-05-05 14:15:02

by Vikash Garodia

[permalink] [raw]
Subject: Re: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()


On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
> This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.
Again, why is it marked as fix ?
>
> Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
> Reviewed-by: Bryan O'Donoghue <[email protected]>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/vdec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 51a53bf82bd3..33e3f7208b1a 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
> u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
> struct hfi_video_work_route wr;
>
> - if (!IS_V6(inst->core))
> + if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))

Not a good idea to add IRIS1 just for deciding work route and not at
other places in driver. Add IRIS1 relevant

code in other aspects as well, if the patch needs to handle anything
w.r.t IRIS1.

> return 0;
>
> wr.video_work_route = inst->core->res->num_vpp_pipes;
>

2023-05-05 19:31:57

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()



On 5.05.2023 16:02, Vikash Garodia wrote:
>
> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>> This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.
> Again, why is it marked as fix ?
It corrects the logic but does not manifest on currently
supported hardware. I'll reword it and drop the fixes tag.

>>
>> Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
>> Reviewed-by: Bryan O'Donoghue <[email protected]>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>>   drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>> index 51a53bf82bd3..33e3f7208b1a 100644
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
>>       u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
>>       struct hfi_video_work_route wr;
>>   -    if (!IS_V6(inst->core))
>> +    if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))
>
> Not a good idea to add IRIS1 just for deciding work route and not at other places in driver. Add IRIS1 relevant
>
> code in other aspects as well, if the patch needs to handle anything w.r.t IRIS1.
I'd say that correcting this condition is fair regardless. I can
however delay this patch until IRIS1 enablement if you'd prefer
that.

Konrad
>
>>           return 0;
>>         wr.video_work_route = inst->core->res->num_vpp_pipes;
>>

2023-05-09 12:42:34

by Vikash Garodia

[permalink] [raw]
Subject: Re: [PATCH v2 15/18] media: venus: vdec: Fix version check in vdec_set_work_route()



On 5/6/2023 12:45 AM, Konrad Dybcio wrote:
>
>
> On 5.05.2023 16:02, Vikash Garodia wrote:
>>
>> On 5/4/2023 1:31 PM, Konrad Dybcio wrote:
>>> This is not so much V6-dependent as it's IRIS(1|2|2_1). Fix it.
>> Again, why is it marked as fix ?
> It corrects the logic but does not manifest on currently
> supported hardware. I'll reword it and drop the fixes tag.
>
>>>
>>> Fixes: 6483a8cbea54 ("media: venus: vdec: set work route to fw")
>>> Reviewed-by: Bryan O'Donoghue <[email protected]>
>>> Signed-off-by: Konrad Dybcio <[email protected]>
>>> ---
>>>   drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>>> index 51a53bf82bd3..33e3f7208b1a 100644
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -701,7 +701,7 @@ static int vdec_set_work_route(struct venus_inst *inst)
>>>       u32 ptype = HFI_PROPERTY_PARAM_WORK_ROUTE;
>>>       struct hfi_video_work_route wr;
>>>   -    if (!IS_V6(inst->core))
>>> +    if (!(IS_IRIS1(inst->core) || IS_IRIS2(inst->core) || IS_IRIS2_1(inst->core)))
>>
>> Not a good idea to add IRIS1 just for deciding work route and not at other places in driver. Add IRIS1 relevant
>>
>> code in other aspects as well, if the patch needs to handle anything w.r.t IRIS1.
> I'd say that correcting this condition is fair regardless. I can
> however delay this patch until IRIS1 enablement if you'd prefer
> that.

Lets add IRIS1 along with other IRIS1 changes.

> Konrad
>>
>>>           return 0;
>>>         wr.video_work_route = inst->core->res->num_vpp_pipes;
>>>