diff mbox series

[4/4] venus: return P010 as preferred format for 10 bit decode

Message ID 1683196599-3730-5-git-send-email-quic_dikshita@quicinc.com
State New
Headers
Series venus: add support for 10 bit decoding |

Commit Message

Dikshita Agarwal May 4, 2023, 10:36 a.m. UTC 
  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(-)

Comments

Konrad Dybcio May 4, 2023, 5:20 p.m. UTC | #1 
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;
Dikshita Agarwal May 5, 2023, 9:03 a.m. UTC | #2 
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;
Konrad Dybcio May 5, 2023, 10:42 a.m. UTC | #3 
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;
Dmitry Baryshkov May 5, 2023, 11:03 a.m. UTC | #4 
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 mbox series

Patch

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;