All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] venus: helper: do not set constrained format for UBWC
@ 2021-05-28  9:48 Mansur Alisha Shaik
  2021-05-28 17:23 ` Bryan O'Donoghue
  0 siblings, 1 reply; 6+ messages in thread
From: Mansur Alisha Shaik @ 2021-05-28  9:48 UTC (permalink / raw)
  To: linux-media, stanimir.varbanov
  Cc: linux-kernel, linux-arm-msm, vgarodia, mansur, dikshita

Do not set constrained format explicitly for UBWC

Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index b813d6dba..e4b8a2a 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1138,8 +1138,12 @@ int venus_helper_set_format_constraints(struct venus_inst *inst)
 	if (!IS_V6(inst->core))
 		return 0;
 
+	if (inst->opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC)
+		return 0;
+
 	pconstraint.buffer_type = HFI_BUFFER_OUTPUT2;
 	pconstraint.num_planes = 2;
+
 	pconstraint.plane_format[0].stride_multiples = 128;
 	pconstraint.plane_format[0].max_stride = 8192;
 	pconstraint.plane_format[0].min_plane_buffer_height_multiple = 32;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] venus: helper: do not set constrained format for UBWC
  2021-05-28  9:48 [PATCH] venus: helper: do not set constrained format for UBWC Mansur Alisha Shaik
@ 2021-05-28 17:23 ` Bryan O'Donoghue
  2021-06-02  9:53   ` Stanimir Varbanov
  0 siblings, 1 reply; 6+ messages in thread
From: Bryan O'Donoghue @ 2021-05-28 17:23 UTC (permalink / raw)
  To: Mansur Alisha Shaik, linux-media, stanimir.varbanov
  Cc: linux-kernel, linux-arm-msm, vgarodia, dikshita

On 28/05/2021 10:48, Mansur Alisha Shaik wrote:
> Do not set constrained format explicitly for UBWC
> 
> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>

Could you give a little bit more detail on why, what the side effects are ?

Should this be a Fixes: ?

> ---
>   drivers/media/platform/qcom/venus/helpers.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index b813d6dba..e4b8a2a 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1138,8 +1138,12 @@ int venus_helper_set_format_constraints(struct venus_inst *inst)
>   	if (!IS_V6(inst->core))
>   		return 0;
>   
> +	if (inst->opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC)
> +		return 0;
> +
>   	pconstraint.buffer_type = HFI_BUFFER_OUTPUT2;
>   	pconstraint.num_planes = 2;
> +

Additional newline should be removed.

>   	pconstraint.plane_format[0].stride_multiples = 128;
>   	pconstraint.plane_format[0].max_stride = 8192;
>   	pconstraint.plane_format[0].min_plane_buffer_height_multiple = 32;
> 

---
bod

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] venus: helper: do not set constrained format for UBWC
  2021-05-28 17:23 ` Bryan O'Donoghue
@ 2021-06-02  9:53   ` Stanimir Varbanov
  2021-06-14  6:25     ` mansur
  0 siblings, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2021-06-02  9:53 UTC (permalink / raw)
  To: Bryan O'Donoghue, Mansur Alisha Shaik, linux-media,
	stanimir.varbanov
  Cc: linux-kernel, linux-arm-msm, vgarodia, dikshita

Mansur, could you answer to Bryan's comments?

On 5/28/21 8:23 PM, Bryan O'Donoghue wrote:
> On 28/05/2021 10:48, Mansur Alisha Shaik wrote:
>> Do not set constrained format explicitly for UBWC
>>
>> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
> 
> Could you give a little bit more detail on why, what the side effects are ?
> 
> Should this be a Fixes: ?
> 
>> ---
>>   drivers/media/platform/qcom/venus/helpers.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>> b/drivers/media/platform/qcom/venus/helpers.c
>> index b813d6dba..e4b8a2a 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -1138,8 +1138,12 @@ int venus_helper_set_format_constraints(struct
>> venus_inst *inst)
>>       if (!IS_V6(inst->core))
>>           return 0;
>>   +    if (inst->opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC)
>> +        return 0;
>> +
>>       pconstraint.buffer_type = HFI_BUFFER_OUTPUT2;
>>       pconstraint.num_planes = 2;
>> +
> 
> Additional newline should be removed.
> 
>>       pconstraint.plane_format[0].stride_multiples = 128;
>>       pconstraint.plane_format[0].max_stride = 8192;
>>       pconstraint.plane_format[0].min_plane_buffer_height_multiple = 32;
>>
> 
> ---
> bod

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] venus: helper: do not set constrained format for UBWC
  2021-06-02  9:53   ` Stanimir Varbanov
@ 2021-06-14  6:25     ` mansur
  2021-06-14 11:56       ` Bryan O'Donoghue
  0 siblings, 1 reply; 6+ messages in thread
From: mansur @ 2021-06-14  6:25 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Bryan O'Donoghue, linux-media, linux-kernel, linux-arm-msm,
	vgarodia, dikshita

On 2021-06-02 15:23, Stanimir Varbanov wrote:
> Mansur, could you answer to Bryan's comments?
> 
>> On 5/28/21 8:23 PM, Bryan O'Donoghue wrote:
>>> On 28/05/2021 10:48, Mansur Alisha Shaik wrote:
>>>> Do not set constrained format explicitly for UBWC
>>>> 
>>>> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
>>> 
>>> Could you give a little bit more detail on why, what the side effects 
>>> are ?
>>> 
	Sorry for late response, by default for NV12_UBWC is 128x32

>>> Should this be a Fixes: ?
>>> 
	without this fix on V6, firmware throws below SFR
	qcom-venus aa00000.video-codec: SFR message from FW: 
QC_IMAGE_VERSION_STRING=video-firmware.1.0-df9cb37cf8e507a4468265658702247652351a49
	Err_Fatal - 
/local/mnt/workspace/pkg/builds/dynamic_timely/tree2/vendor/qcom/proprietary/video-firmware-noship/venus_proc/venus/decoders/common/src/video_decoder.c:6644:2fef3
	which result in playback happens through Software codec.
>>>> ---
>>>>   drivers/media/platform/qcom/venus/helpers.c | 4 ++++
>>>>   1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>>>> b/drivers/media/platform/qcom/venus/helpers.c
>>>> index b813d6dba..e4b8a2a 100644
>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>>>> @@ -1138,8 +1138,12 @@ int 
>>>> venus_helper_set_format_constraints(struct
>>>> venus_inst *inst)
>>>>       if (!IS_V6(inst->core))
>>>>           return 0;
>>>>   +    if (inst->opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC)
>>>> +        return 0;
>>>> +
>>>>       pconstraint.buffer_type = HFI_BUFFER_OUTPUT2;
>>>>       pconstraint.num_planes = 2;
>>>> +
>>> 
>>> Additional newline should be removed.
>>> 
>>>>       pconstraint.plane_format[0].stride_multiples = 128;
>>>>       pconstraint.plane_format[0].max_stride = 8192;
>>>>       pconstraint.plane_format[0].min_plane_buffer_height_multiple = 
>>>> 32;
>>>> 
>>> 
>>> ---
>>> bod

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] venus: helper: do not set constrained format for UBWC
  2021-06-14  6:25     ` mansur
@ 2021-06-14 11:56       ` Bryan O'Donoghue
  2021-06-19  5:52         ` vgarodia
  0 siblings, 1 reply; 6+ messages in thread
From: Bryan O'Donoghue @ 2021-06-14 11:56 UTC (permalink / raw)
  To: mansur, Stanimir Varbanov
  Cc: linux-media, linux-kernel, linux-arm-msm, vgarodia, dikshita

On 14/06/2021 07:25, mansur@codeaurora.org wrote:
> On 2021-06-02 15:23, Stanimir Varbanov wrote:
>> Mansur, could you answer to Bryan's comments?
>>
>>> On 5/28/21 8:23 PM, Bryan O'Donoghue wrote:
>>>> On 28/05/2021 10:48, Mansur Alisha Shaik wrote:
>>>>> Do not set constrained format explicitly for UBWC
>>>>>
>>>>> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
>>>>
>>>> Could you give a little bit more detail on why, what the side 
>>>> effects are ?
>>>>
>      Sorry for late response, by default for NV12_UBWC is 128x32
> 

Right so we have

pconstraint.plane_format[0].stride_multiples = 128;
pconstraint.plane_format[0].min_plane_buffer_height_multiple = 32;

and

pconstraint.plane_format[1].stride_multiples = 128;
pconstraint.plane_format[1].min_plane_buffer_height_multiple = 16;

and your patch says if opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC then the we 
shouldn't do hfi_session_set_property()

I'm sure that's a fix that works but, I wonder would it be possible to 
fix this routine to continue to do hfi_session_set_property() with 
updated parameters for opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC ?

Could you drill down into the detail in the commit log a little bit 
maybe giving a description of why returning for opb_fmt == 
HFI_COLOR_FORMAT_NV12_UBWC is the right thing to-do instead of adding a 
new case to the routine for HFI_COLOR_FORMAT_NV12_UBWC and calling 
hfi_session_set_property() ?

Its more for my own education on this topic :) but, also helps somebody 
else reading the log to understand what the fix is, why it is done this way.

>>>> Should this be a Fixes: ?
>>>>
>      without this fix on V6, firmware throws below SFR
>      qcom-venus aa00000.video-codec: SFR message from FW: 
> QC_IMAGE_VERSION_STRING=video-firmware.1.0-df9cb37cf8e507a4468265658702247652351a49 
> 
>      Err_Fatal - 
> /local/mnt/workspace/pkg/builds/dynamic_timely/tree2/vendor/qcom/proprietary/video-firmware-noship/venus_proc/venus/decoders/common/src/video_decoder.c:6644:2fef3 
> 
>      which result in playback happens through Software codec.
OK, I think I can answer my own question here.

Technically no since 6XX isn't in the long-term-support kernel but, I'd 
suggest adding a "Fixes" anyway, so that other users know to apply this 
patch to their trees.

Fixes: bc28936bbba9 ("media: venus: helpers, hfi, vdec: Set actual plane 
constraints to FW")

---
bod

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] venus: helper: do not set constrained format for UBWC
  2021-06-14 11:56       ` Bryan O'Donoghue
@ 2021-06-19  5:52         ` vgarodia
  0 siblings, 0 replies; 6+ messages in thread
From: vgarodia @ 2021-06-19  5:52 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: mansur, Stanimir Varbanov, linux-media, linux-kernel,
	linux-arm-msm, dikshita

Hi Bryan,

On 2021-06-14 17:26, Bryan O'Donoghue wrote:
> On 14/06/2021 07:25, mansur@codeaurora.org wrote:
>> On 2021-06-02 15:23, Stanimir Varbanov wrote:
>>> Mansur, could you answer to Bryan's comments?
>>> 
>>>> On 5/28/21 8:23 PM, Bryan O'Donoghue wrote:
>>>>> On 28/05/2021 10:48, Mansur Alisha Shaik wrote:
>>>>>> Do not set constrained format explicitly for UBWC
>>>>>> 
>>>>>> Signed-off-by: Mansur Alisha Shaik <mansur@codeaurora.org>
>>>>> 
>>>>> Could you give a little bit more detail on why, what the side 
>>>>> effects are ?
>>>>> 
>>      Sorry for late response, by default for NV12_UBWC is 128x32
>> 
> 
> Right so we have
> 
> pconstraint.plane_format[0].stride_multiples = 128;
> pconstraint.plane_format[0].min_plane_buffer_height_multiple = 32;
> 
> and
> 
> pconstraint.plane_format[1].stride_multiples = 128;
> pconstraint.plane_format[1].min_plane_buffer_height_multiple = 16;
> 
> and your patch says if opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC then the
> we shouldn't do hfi_session_set_property()
> 
> I'm sure that's a fix that works but, I wonder would it be possible to
> fix this routine to continue to do hfi_session_set_property() with
> updated parameters for opb_fmt == HFI_COLOR_FORMAT_NV12_UBWC ?

Venus hardware would always go with alignments as 128x32 for WxH 
irrespective
of color formats. It happened so (historically) that for NV12 (linear) 
format,
usecase started demanding for alignments as 256(interlace) and 
512(HEIF), and
hence NV12 was defaulted to align as 512x512 in firmware. This was done 
to avoid
carrying multiple alignments for different usecases, since aligning with 
512x512
would also align it with 128x32 and 256x256 as well.
For UBWC, there is no need to override the default alignment of 128x32, 
hence
the api was added to override default alignments for applicable formats, 
in
this case NV12(Linear).

> Could you drill down into the detail in the commit log a little bit
> maybe giving a description of why returning for opb_fmt ==
> HFI_COLOR_FORMAT_NV12_UBWC is the right thing to-do instead of adding
> a new case to the routine for HFI_COLOR_FORMAT_NV12_UBWC and calling
> hfi_session_set_property() ?

Above details should provide info on the need to set this only for NV12 
(linear)
and skip for NV12 (UBWC).

> Its more for my own education on this topic :) but, also helps
> somebody else reading the log to understand what the fix is, why it is
> done this way.
> 
>>>>> Should this be a Fixes: ?
>>>>> 
>>      without this fix on V6, firmware throws below SFR
>>      qcom-venus aa00000.video-codec: SFR message from FW: 
>> QC_IMAGE_VERSION_STRING=video-firmware.1.0-df9cb37cf8e507a4468265658702247652351a49 
>>      Err_Fatal - 
>> /local/mnt/workspace/pkg/builds/dynamic_timely/tree2/vendor/qcom/proprietary/video-firmware-noship/venus_proc/venus/decoders/common/src/video_decoder.c:6644:2fef3 
>>      which result in playback happens through Software codec.
> OK, I think I can answer my own question here.
> 
> Technically no since 6XX isn't in the long-term-support kernel but,
> I'd suggest adding a "Fixes" anyway, so that other users know to apply
> this patch to their trees.
> 
> Fixes: bc28936bbba9 ("media: venus: helpers, hfi, vdec: Set actual
> plane constraints to FW")

Yes, it should go as "Fixes" tag to above patch which we made for 6xx 
recently.

Thanks,
Vikash

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-19  5:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-28  9:48 [PATCH] venus: helper: do not set constrained format for UBWC Mansur Alisha Shaik
2021-05-28 17:23 ` Bryan O'Donoghue
2021-06-02  9:53   ` Stanimir Varbanov
2021-06-14  6:25     ` mansur
2021-06-14 11:56       ` Bryan O'Donoghue
2021-06-19  5:52         ` vgarodia

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.