Re: [PATCH] venus: helper: do not set constrained format for UBWC

From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
To: mansur@codeaurora.org, Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, vgarodia@codeaurora.org,
	dikshita@codeaurora.org
Subject: Re: [PATCH] venus: helper: do not set constrained format for UBWC
Date: Mon, 14 Jun 2021 12:56:51 +0100	[thread overview]
Message-ID: <28ab2706-2a0c-e9d6-c02c-8ede6fa01efb@linaro.org> (raw)
In-Reply-To: <b649a7ecb3feb1fdf6d0743135814840@codeaurora.org>

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