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

[Bryan O'Donoghue]

On 14/06/2021 07:25, mansur@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx>
> > > >
> > > > 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