[PATCH 0/2] media: venus: Fix up buffer handling for HFI_VERSION_1XX

[PATCH 0/2] media: venus: Fix up buffer handling for HFI_VERSION_1XX

[Bryan O'Donoghue]

This series fixes two buffer handling bugs. The first bug is trivial and
~impossible to hit without the second bug but with fixing anyway. Its a
simple NULL pointer handling issue.

The second bug relates to HFI_VERSION_1XX output buffers. Unfortunately
these have been broken since

Commit: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")

A bit of bisecting the commits in venus found it handily enough. Once the
fix is applied we have I have decode working again on db410c.

Bryan O'Donoghue (2):
  media: venus: dec: Handle the case where find_format fails
  media: venus: Fix NV12 decoder buffer discovery on HFI_VERSION_1XX

 drivers/media/platform/qcom/venus/helpers.c | 13 +++++++------
 drivers/media/platform/qcom/venus/vdec.c    |  2 ++
 2 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.36.1

[PATCH 1/2] media: venus: dec: Handle the case where find_format fails

[Bryan O'Donoghue]

Debugging the decoder on msm8916 I noticed the vdec probe was crashing if
the fmt pointer was NULL.

A similar fix from Colin Ian King found by Coverity was implemented for the
encoder. Implement the same fix on the decoder.

Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index ac0bb45d07f4b..4ceaba37e2e57 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -183,6 +183,8 @@ vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
 		else
 			return NULL;
 		fmt = find_format(inst, pixmp->pixelformat, f->type);
+		if (!fmt)
+			return NULL;
 	}
 
 	pixmp->width = clamp(pixmp->width, frame_width_min(inst),
-- 
2.36.1

[PATCH 2/2] media: venus: Fix NV12 decoder buffer discovery on HFI_VERSION_1XX

[Bryan O'Donoghue]

HFI_VERSION_1XX uses HFI_BUFFER_OUTPUT not HFI_BUFFER_OUTPUT2 for decoder
buffers.

venus_helper_check_format() places a constraint on an output buffer to be
of type HFI_BUFFER_OUTPUT2. HFI_1XX uses HFI_BUFFER_OUTPUT though.

Switching to the logic used in venus_helper_get_out_fmts() first checking
for HFI_BUFFER_OUTPUT and then HFI_BUFFER_OUTPUT2 resolves on HFI_1XX.

db410c before:
root@linaro-alip:~# v4l2-ctl  -d /dev/video0 --list-formats
ioctl: VIDIOC_ENUM_FMT
        Type: Video Capture Multiplanar

        [0]: 'MPG4' (MPEG-4 Part 2 ES, compressed)
        [1]: 'H263' (H.263, compressed)
        [2]: 'H264' (H.264, compressed)
        [3]: 'VP80' (VP8, compressed)

root@linaro-alip:~# v4l2-ctl  -d /dev/video1 --list-formats
ioctl: VIDIOC_ENUM_FMT
        Type: Video Capture Multiplanar

db410c after:
root@linaro-alip:~# v4l2-ctl  -d /dev/video0 --list-formats
ioctl: VIDIOC_ENUM_FMT
        Type: Video Capture Multiplanar

        [0]: 'MPG4' (MPEG-4 Part 2 ES, compressed)
        [1]: 'H263' (H.263, compressed)
        [2]: 'H264' (H.264, compressed)
        [3]: 'VP80' (VP8, compressed)

root@linaro-alip:~# v4l2-ctl  -d /dev/video1 --list-formats
ioctl: VIDIOC_ENUM_FMT
        Type: Video Capture Multiplanar

        [0]: 'NV12' (Y/CbCr 4:2:0)

Validated playback with ffplay on db410c with h264 and vp8 decoding.

Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 5c1104379c491..623be6393c706 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1802,7 +1802,7 @@ bool venus_helper_check_format(struct venus_inst *inst, u32 v4l2_pixfmt)
 	struct venus_core *core = inst->core;
 	u32 fmt = to_hfi_raw_fmt(v4l2_pixfmt);
 	struct hfi_plat_caps *caps;
-	u32 buftype;
+	bool found;
 
 	if (!fmt)
 		return false;
@@ -1811,12 +1811,13 @@ bool venus_helper_check_format(struct venus_inst *inst, u32 v4l2_pixfmt)
 	if (!caps)
 		return false;
 
-	if (inst->session_type == VIDC_SESSION_TYPE_DEC)
-		buftype = HFI_BUFFER_OUTPUT2;
-	else
-		buftype = HFI_BUFFER_OUTPUT;
+	found = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT, fmt);
+	if (found)
+		goto done;
 
-	return find_fmt_from_caps(caps, buftype, fmt);
+	found = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT2, fmt);
+done:
+	return found;
 }
 EXPORT_SYMBOL_GPL(venus_helper_check_format);
 
-- 
2.36.1

Re: [PATCH 0/2] media: venus: Fix up buffer handling for HFI_VERSION_1XX

[Bryan O'Donoghue]

On 26/07/2022 03:14, Bryan O'Donoghue wrote:
> This series fixes two buffer handling bugs. The first bug is trivial and
> ~impossible to hit without the second bug but with fixing anyway. Its a
> simple NULL pointer handling issue.
> 
> The second bug relates to HFI_VERSION_1XX output buffers. Unfortunately
> these have been broken since
> 
> Commit: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
> 
> A bit of bisecting the commits in venus found it handily enough. Once the
> fix is applied we have I have decode working again on db410c.
> 
> Bryan O'Donoghue (2):
>    media: venus: dec: Handle the case where find_format fails
>    media: venus: Fix NV12 decoder buffer discovery on HFI_VERSION_1XX
> 
>   drivers/media/platform/qcom/venus/helpers.c | 13 +++++++------
>   drivers/media/platform/qcom/venus/vdec.c    |  2 ++
>   2 files changed, 9 insertions(+), 6 deletions(-)
> 

Not withstanding my 3am misspelled text above, I've also just validated 
this changes on RB5/HFI_VERSION_6xx.

---
bod

Re: [PATCH 0/2] media: venus: Fix up buffer handling for HFI_VERSION_1XX

[Stanimir Varbanov]

Hi Bryan,

Thanks for the patches!

On 7/26/22 05:14, Bryan O'Donoghue wrote:
> This series fixes two buffer handling bugs. The first bug is trivial and
> ~impossible to hit without the second bug but with fixing anyway. Its a
> simple NULL pointer handling issue.
> 
> The second bug relates to HFI_VERSION_1XX output buffers. Unfortunately
> these have been broken since
> 
> Commit: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")
> 
> A bit of bisecting the commits in venus found it handily enough. Once the
> fix is applied we have I have decode working again on db410c.
> 
> Bryan O'Donoghue (2):
>   media: venus: dec: Handle the case where find_format fails
>   media: venus: Fix NV12 decoder buffer discovery on HFI_VERSION_1XX
> 
>  drivers/media/platform/qcom/venus/helpers.c | 13 +++++++------
>  drivers/media/platform/qcom/venus/vdec.c    |  2 ++
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 

Looks good to me.

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

-- 
regards,
Stan

Re: [PATCH 2/2] media: venus: Fix NV12 decoder buffer discovery on HFI_VERSION_1XX

[Stanimir Varbanov]

On 7/26/22 05:14, Bryan O'Donoghue wrote:
> HFI_VERSION_1XX uses HFI_BUFFER_OUTPUT not HFI_BUFFER_OUTPUT2 for decoder
> buffers.
> 
> venus_helper_check_format() places a constraint on an output buffer to be
> of type HFI_BUFFER_OUTPUT2. HFI_1XX uses HFI_BUFFER_OUTPUT though.
> 

<cut>

> 
> Validated playback with ffplay on db410c with h264 and vp8 decoding.
> 
> Fixes: 9593126dae3e ("media: venus: Add a handling of QC08C compressed format")

Cc: stable@vger.kernel.org  # v5.19

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 


-- 
regards,
Stan

Re: [PATCH 1/2] media: venus: dec: Handle the case where find_format fails

[Stanimir Varbanov]

Hi Bryan,

You forgot to CC: stable.

On 7/26/22 05:14, Bryan O'Donoghue wrote:
> Debugging the decoder on msm8916 I noticed the vdec probe was crashing if
> the fmt pointer was NULL.
> 
> A similar fix from Colin Ian King found by Coverity was implemented for the
> encoder. Implement the same fix on the decoder.
> 
> Fixes: 7472c1c69138 ("[media] media: venus: vdec: add video decoder files")

Cc: stable@vger.kernel.org  # v4.13+

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index ac0bb45d07f4b..4ceaba37e2e57 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -183,6 +183,8 @@ vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
>  		else
>  			return NULL;
>  		fmt = find_format(inst, pixmp->pixelformat, f->type);
> +		if (!fmt)
> +			return NULL;
>  	}
>  
>  	pixmp->width = clamp(pixmp->width, frame_width_min(inst),

-- 
regards,
Stan