| Message ID | 20230228-topic-venus-v2-15-d95d14949c79@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Venus QoL / maintainability fixes | expand |
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 <bryan.odonoghue@linaro.org> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > 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; >
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 <bryan.odonoghue@linaro.org> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> 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; >>
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 <bryan.odonoghue@linaro.org> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>> --- >>> 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; >>>
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;
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 <bryan.odonoghue@linaro.org> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/media/platform/qcom/venus/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)