| Message ID | 1683196599-3730-5-git-send-email-quic_dikshita@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | venus: add support for 10 bit decoding | expand |
On 4.05.2023 12:36, Dikshita Agarwal wrote: > If bit depth is detected as 10 bit by firmware, return > P010 as preferred decoder format to the client. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > drivers/media/platform/qcom/venus/vdec.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c > index 69f7f6e..ed11dc2 100644 > --- a/drivers/media/platform/qcom/venus/vdec.c > +++ b/drivers/media/platform/qcom/venus/vdec.c > @@ -1468,8 +1468,13 @@ static void vdec_event_change(struct venus_inst *inst, > inst->out_width = ev_data->width; > inst->out_height = ev_data->height; > > - if (inst->bit_depth != ev_data->bit_depth) > + if (inst->bit_depth != ev_data->bit_depth) { > inst->bit_depth = ev_data->bit_depth; > + if (inst->bit_depth == VIDC_BITDEPTH_10) > + inst->fmt_cap = &vdec_formats[3]; > + else > + inst->fmt_cap = &vdec_formats[0]; This doesn't scale and is very error-prone, please enumerate the entries and assign it using the enumerator, like: enum { VDEC_FORMAT_FOO, ... }; ... vdec_formats[] = { [VDEC_FORMAT_FOO] = { foo, bar, baz } } Konrad > + } > > if (inst->pic_struct != ev_data->pic_struct) > inst->pic_struct = ev_data->pic_struct;
On 5/4/2023 10:50 PM, Konrad Dybcio wrote: > > On 4.05.2023 12:36, Dikshita Agarwal wrote: >> If bit depth is detected as 10 bit by firmware, return >> P010 as preferred decoder format to the client. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> --- >> drivers/media/platform/qcom/venus/vdec.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >> index 69f7f6e..ed11dc2 100644 >> --- a/drivers/media/platform/qcom/venus/vdec.c >> +++ b/drivers/media/platform/qcom/venus/vdec.c >> @@ -1468,8 +1468,13 @@ static void vdec_event_change(struct venus_inst *inst, >> inst->out_width = ev_data->width; >> inst->out_height = ev_data->height; >> >> - if (inst->bit_depth != ev_data->bit_depth) >> + if (inst->bit_depth != ev_data->bit_depth) { >> inst->bit_depth = ev_data->bit_depth; >> + if (inst->bit_depth == VIDC_BITDEPTH_10) >> + inst->fmt_cap = &vdec_formats[3]; >> + else >> + inst->fmt_cap = &vdec_formats[0]; > This doesn't scale and is very error-prone, please enumerate the > entries and assign it using the enumerator, like: > > enum { > VDEC_FORMAT_FOO, > ... > }; > > ... vdec_formats[] = { > [VDEC_FORMAT_FOO] = { foo, bar, baz } > } > > Konrad I agree, this can be improved but I would prefer making that change as separate patch. As this is not only related to HDR 10 decoding, there are other places in the code which will require similar change. Thanks, Dikshita >> + } >> >> if (inst->pic_struct != ev_data->pic_struct) >> inst->pic_struct = ev_data->pic_struct;
On 5.05.2023 11:03, Dikshita Agarwal wrote: > > On 5/4/2023 10:50 PM, Konrad Dybcio wrote: >> >> On 4.05.2023 12:36, Dikshita Agarwal wrote: >>> If bit depth is detected as 10 bit by firmware, return >>> P010 as preferred decoder format to the client. >>> >>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >>> --- >>> drivers/media/platform/qcom/venus/vdec.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c >>> index 69f7f6e..ed11dc2 100644 >>> --- a/drivers/media/platform/qcom/venus/vdec.c >>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>> @@ -1468,8 +1468,13 @@ static void vdec_event_change(struct venus_inst *inst, >>> inst->out_width = ev_data->width; >>> inst->out_height = ev_data->height; >>> - if (inst->bit_depth != ev_data->bit_depth) >>> + if (inst->bit_depth != ev_data->bit_depth) { >>> inst->bit_depth = ev_data->bit_depth; >>> + if (inst->bit_depth == VIDC_BITDEPTH_10) >>> + inst->fmt_cap = &vdec_formats[3]; >>> + else >>> + inst->fmt_cap = &vdec_formats[0]; >> This doesn't scale and is very error-prone, please enumerate the >> entries and assign it using the enumerator, like: >> >> enum { >> VDEC_FORMAT_FOO, >> ... >> }; >> >> ... vdec_formats[] = { >> [VDEC_FORMAT_FOO] = { foo, bar, baz } >> } >> >> Konrad > > I agree, this can be improved but I would prefer making that change as separate patch. > > As this is not only related to HDR 10 decoding, there are other places in the code which will require similar change. This is not a very strong argument. You can't add code that will break very easily whenever somebody touches that array and pinky-promise to fix it some time later, just because you want to get your feature merged faster, this is not drivers/staging. Konrad > > Thanks, > > Dikshita > >>> + } >>> if (inst->pic_struct != ev_data->pic_struct) >>> inst->pic_struct = ev_data->pic_struct;
On 05/05/2023 12:03, Dikshita Agarwal wrote: > > On 5/4/2023 10:50 PM, Konrad Dybcio wrote: >> >> On 4.05.2023 12:36, Dikshita Agarwal wrote: >>> If bit depth is detected as 10 bit by firmware, return >>> P010 as preferred decoder format to the client. >>> >>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >>> --- >>> drivers/media/platform/qcom/venus/vdec.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/vdec.c >>> b/drivers/media/platform/qcom/venus/vdec.c >>> index 69f7f6e..ed11dc2 100644 >>> --- a/drivers/media/platform/qcom/venus/vdec.c >>> +++ b/drivers/media/platform/qcom/venus/vdec.c >>> @@ -1468,8 +1468,13 @@ static void vdec_event_change(struct >>> venus_inst *inst, >>> inst->out_width = ev_data->width; >>> inst->out_height = ev_data->height; >>> - if (inst->bit_depth != ev_data->bit_depth) >>> + if (inst->bit_depth != ev_data->bit_depth) { >>> inst->bit_depth = ev_data->bit_depth; >>> + if (inst->bit_depth == VIDC_BITDEPTH_10) >>> + inst->fmt_cap = &vdec_formats[3]; >>> + else >>> + inst->fmt_cap = &vdec_formats[0]; >> This doesn't scale and is very error-prone, please enumerate the >> entries and assign it using the enumerator, like: >> >> enum { >> VDEC_FORMAT_FOO, >> ... >> }; >> >> ... vdec_formats[] = { >> [VDEC_FORMAT_FOO] = { foo, bar, baz } >> } >> >> Konrad > > I agree, this can be improved but I would prefer making that change as > separate patch. Good! > > As this is not only related to HDR 10 decoding, there are other places > in the code which will require similar change. Please fix them first. Adding more cruft is not a good way to go. > > Thanks, > > Dikshita > >>> + } >>> if (inst->pic_struct != ev_data->pic_struct) >>> inst->pic_struct = ev_data->pic_struct;
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 69f7f6e..ed11dc2 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -1468,8 +1468,13 @@ static void vdec_event_change(struct venus_inst *inst, inst->out_width = ev_data->width; inst->out_height = ev_data->height; - if (inst->bit_depth != ev_data->bit_depth) + if (inst->bit_depth != ev_data->bit_depth) { inst->bit_depth = ev_data->bit_depth; + if (inst->bit_depth == VIDC_BITDEPTH_10) + inst->fmt_cap = &vdec_formats[3]; + else + inst->fmt_cap = &vdec_formats[0]; + } if (inst->pic_struct != ev_data->pic_struct) inst->pic_struct = ev_data->pic_struct;
If bit depth is detected as 10 bit by firmware, return P010 as preferred decoder format to the client. Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> --- drivers/media/platform/qcom/venus/vdec.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)