[PATCH 0/4] venus: add support for 10 bit decoding

[PATCH 0/4] venus: add support for 10 bit decoding

[Dikshita Agarwal]

This series includes the changes to
  - add V4L2_PIX_FMT_P010 as supported decoder format.
  - consider dpb color format while calculating buffer
    size for dpb buffers.
  - update dpb and opb color format when bit depth
    changes is detected, also update preferred color
    format to P010 in this case.

With this series, divided the previous version [1] into
multiple patches as suggested in review comments.

[1] https://patchwork.linuxtv.org/project/linux-media/list/?series=10376

Dikshita Agarwal (4):
  venus: add support for V4L2_PIX_FMT_P010 color format
  venus: update calculation for dpb buffers
  venus: add handling of bit depth change from firmwar
  venus: return P010 as preferred format for 10 bit decode

 drivers/media/platform/qcom/venus/helpers.c        | 24 ++++++++++++++++++++++
 drivers/media/platform/qcom/venus/hfi_plat_bufs.h  |  3 +++
 .../media/platform/qcom/venus/hfi_plat_bufs_v6.c   |  8 +++++++-
 drivers/media/platform/qcom/venus/vdec.c           | 16 +++++++++++++--
 4 files changed, 48 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/4] venus: add support for V4L2_PIX_FMT_P010 color format

[Dikshita Agarwal]

add V4L2_PIX_FMT_P010 as supported color format for decoder.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 ++
 drivers/media/platform/qcom/venus/vdec.c    | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index ab6a29f..5946def 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -612,6 +612,8 @@ static u32 to_hfi_raw_fmt(u32 v4l2_fmt)
 		return HFI_COLOR_FORMAT_NV12_UBWC;
 	case V4L2_PIX_FMT_QC10C:
 		return HFI_COLOR_FORMAT_YUV420_TP10_UBWC;
+	case V4L2_PIX_FMT_P010:
+		return HFI_COLOR_FORMAT_P010;
 	default:
 		break;
 	}
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 4ceaba3..687d62e 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -43,6 +43,10 @@ static const struct venus_format vdec_formats[] = {
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
 	}, {
+		.pixfmt = V4L2_PIX_FMT_P010,
+		.num_planes = 1,
+		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
+	}, {
 		.pixfmt = V4L2_PIX_FMT_MPEG4,
 		.num_planes = 1,
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
-- 
2.7.4

Re: [PATCH 1/4] venus: add support for V4L2_PIX_FMT_P010 color format

[Konrad Dybcio]

On 4.05.2023 12:36, Dikshita Agarwal wrote:
> add V4L2_PIX_FMT_P010 as supported color format for decoder.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/media/platform/qcom/venus/helpers.c | 2 ++
>  drivers/media/platform/qcom/venus/vdec.c    | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index ab6a29f..5946def 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -612,6 +612,8 @@ static u32 to_hfi_raw_fmt(u32 v4l2_fmt)
>  		return HFI_COLOR_FORMAT_NV12_UBWC;
>  	case V4L2_PIX_FMT_QC10C:
>  		return HFI_COLOR_FORMAT_YUV420_TP10_UBWC;
> +	case V4L2_PIX_FMT_P010:
> +		return HFI_COLOR_FORMAT_P010;
>  	default:
>  		break;
>  	}
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 4ceaba3..687d62e 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -43,6 +43,10 @@ static const struct venus_format vdec_formats[] = {
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
>  	}, {
> +		.pixfmt = V4L2_PIX_FMT_P010,
> +		.num_planes = 1,
> +		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> +	}, {
>  		.pixfmt = V4L2_PIX_FMT_MPEG4,
>  		.num_planes = 1,
>  		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,

[PATCH 2/4] venus: update calculation for dpb buffers

[Dikshita Agarwal]

Use dpb color format, width and height of output port
for calculating buffer size of dpb buffers.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/helpers.c          | 4 ++++
 drivers/media/platform/qcom/venus/hfi_plat_bufs.h    | 3 +++
 drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c | 8 +++++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 5946def..4ad6232 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -641,12 +641,16 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
 	if (is_dec) {
 		params.width = inst->width;
 		params.height = inst->height;
+		params.out_width = inst->out_width;
+		params.out_height = inst->out_height;
 		params.codec = inst->fmt_out->pixfmt;
 		params.hfi_color_fmt = to_hfi_raw_fmt(inst->fmt_cap->pixfmt);
 		params.dec.max_mbs_per_frame = mbs_per_frame_max(inst);
 		params.dec.buffer_size_limit = 0;
 		params.dec.is_secondary_output =
 			inst->opb_buftype == HFI_BUFFER_OUTPUT2;
+		if (params.dec.is_secondary_output)
+			params.hfi_dpb_color_fmt = inst->dpb_fmt;
 		params.dec.is_interlaced =
 			inst->pic_struct != HFI_INTERLACE_FRAME_PROGRESSIVE;
 	} else {
diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs.h b/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
index 52a51a3..25e6074 100644
--- a/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
+++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
@@ -12,8 +12,11 @@
 struct hfi_plat_buffers_params {
 	u32 width;
 	u32 height;
+	u32 out_width;
+	u32 out_height;
 	u32 codec;
 	u32 hfi_color_fmt;
+	u32 hfi_dpb_color_fmt;
 	enum hfi_version version;
 	u32 num_vpp_pipes;
 	union {
diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
index ea25c45..3855b04 100644
--- a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
+++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
@@ -1185,6 +1185,7 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
 	enum hfi_version version = params->version;
 	u32 codec = params->codec;
 	u32 width = params->width, height = params->height, out_min_count;
+	u32 out_width = params->out_width, out_height = params->out_height;
 	struct dec_bufsize_ops *dec_ops;
 	bool is_secondary_output = params->dec.is_secondary_output;
 	bool is_interlaced = params->dec.is_interlaced;
@@ -1235,7 +1236,12 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
 		bufreq->count_min = out_min_count;
 		bufreq->size =
 			venus_helper_get_framesz_raw(params->hfi_color_fmt,
-						     width, height);
+						     out_width, out_height);
+		if (buftype == HFI_BUFFER_OUTPUT &&
+		    params->dec.is_secondary_output)
+			bufreq->size =
+				venus_helper_get_framesz_raw(params->hfi_dpb_color_fmt,
+							     out_width, out_height);
 	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH(version)) {
 		bufreq->size = dec_ops->scratch(width, height, is_interlaced);
 	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH_1(version)) {
-- 
2.7.4

Re: [PATCH 2/4] venus: update calculation for dpb buffers

[Konrad Dybcio]

On 4.05.2023 12:36, Dikshita Agarwal wrote:
> Use dpb color format, width and height of output port
> for calculating buffer size of dpb buffers.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
Looks sane but I'm not exactly an expert on this

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/media/platform/qcom/venus/helpers.c          | 4 ++++
>  drivers/media/platform/qcom/venus/hfi_plat_bufs.h    | 3 +++
>  drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c | 8 +++++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 5946def..4ad6232 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -641,12 +641,16 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
>  	if (is_dec) {
>  		params.width = inst->width;
>  		params.height = inst->height;
> +		params.out_width = inst->out_width;
> +		params.out_height = inst->out_height;
>  		params.codec = inst->fmt_out->pixfmt;
>  		params.hfi_color_fmt = to_hfi_raw_fmt(inst->fmt_cap->pixfmt);
>  		params.dec.max_mbs_per_frame = mbs_per_frame_max(inst);
>  		params.dec.buffer_size_limit = 0;
>  		params.dec.is_secondary_output =
>  			inst->opb_buftype == HFI_BUFFER_OUTPUT2;
> +		if (params.dec.is_secondary_output)
> +			params.hfi_dpb_color_fmt = inst->dpb_fmt;
>  		params.dec.is_interlaced =
>  			inst->pic_struct != HFI_INTERLACE_FRAME_PROGRESSIVE;
>  	} else {
> diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs.h b/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
> index 52a51a3..25e6074 100644
> --- a/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
> +++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
> @@ -12,8 +12,11 @@
>  struct hfi_plat_buffers_params {
>  	u32 width;
>  	u32 height;
> +	u32 out_width;
> +	u32 out_height;
>  	u32 codec;
>  	u32 hfi_color_fmt;
> +	u32 hfi_dpb_color_fmt;
>  	enum hfi_version version;
>  	u32 num_vpp_pipes;
>  	union {
> diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
> index ea25c45..3855b04 100644
> --- a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
> +++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
> @@ -1185,6 +1185,7 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
>  	enum hfi_version version = params->version;
>  	u32 codec = params->codec;
>  	u32 width = params->width, height = params->height, out_min_count;
> +	u32 out_width = params->out_width, out_height = params->out_height;
>  	struct dec_bufsize_ops *dec_ops;
>  	bool is_secondary_output = params->dec.is_secondary_output;
>  	bool is_interlaced = params->dec.is_interlaced;
> @@ -1235,7 +1236,12 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
>  		bufreq->count_min = out_min_count;
>  		bufreq->size =
>  			venus_helper_get_framesz_raw(params->hfi_color_fmt,
> -						     width, height);
> +						     out_width, out_height);
> +		if (buftype == HFI_BUFFER_OUTPUT &&
> +		    params->dec.is_secondary_output)
> +			bufreq->size =
> +				venus_helper_get_framesz_raw(params->hfi_dpb_color_fmt,
> +							     out_width, out_height);
>  	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH(version)) {
>  		bufreq->size = dec_ops->scratch(width, height, is_interlaced);
>  	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH_1(version)) {

Re: [PATCH 2/4] venus: update calculation for dpb buffers

[Konrad Dybcio]

On 4.05.2023 19:29, Konrad Dybcio wrote:
> 
> 
> On 4.05.2023 12:36, Dikshita Agarwal wrote:
>> Use dpb color format, width and height of output port
>> for calculating buffer size of dpb buffers.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
> Looks sane but I'm not exactly an expert on this
> 
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Konrad
>>  drivers/media/platform/qcom/venus/helpers.c          | 4 ++++
>>  drivers/media/platform/qcom/venus/hfi_plat_bufs.h    | 3 +++
>>  drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c | 8 +++++++-
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
>> index 5946def..4ad6232 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -641,12 +641,16 @@ static int platform_get_bufreq(struct venus_inst *inst, u32 buftype,
>>  	if (is_dec) {
>>  		params.width = inst->width;
>>  		params.height = inst->height;
>> +		params.out_width = inst->out_width;
>> +		params.out_height = inst->out_height;
>>  		params.codec = inst->fmt_out->pixfmt;
>>  		params.hfi_color_fmt = to_hfi_raw_fmt(inst->fmt_cap->pixfmt);
>>  		params.dec.max_mbs_per_frame = mbs_per_frame_max(inst);
>>  		params.dec.buffer_size_limit = 0;
>>  		params.dec.is_secondary_output =
>>  			inst->opb_buftype == HFI_BUFFER_OUTPUT2;
>> +		if (params.dec.is_secondary_output)
>> +			params.hfi_dpb_color_fmt = inst->dpb_fmt;
>>  		params.dec.is_interlaced =
>>  			inst->pic_struct != HFI_INTERLACE_FRAME_PROGRESSIVE;
>>  	} else {
>> diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs.h b/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
>> index 52a51a3..25e6074 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
>> +++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs.h
>> @@ -12,8 +12,11 @@
>>  struct hfi_plat_buffers_params {
>>  	u32 width;
>>  	u32 height;
>> +	u32 out_width;
>> +	u32 out_height;
>>  	u32 codec;
>>  	u32 hfi_color_fmt;
>> +	u32 hfi_dpb_color_fmt;
>>  	enum hfi_version version;
>>  	u32 num_vpp_pipes;
>>  	union {
>> diff --git a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
>> index ea25c45..3855b04 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_plat_bufs_v6.c
>> @@ -1185,6 +1185,7 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
>>  	enum hfi_version version = params->version;
>>  	u32 codec = params->codec;
>>  	u32 width = params->width, height = params->height, out_min_count;
>> +	u32 out_width = params->out_width, out_height = params->out_height;
>>  	struct dec_bufsize_ops *dec_ops;
>>  	bool is_secondary_output = params->dec.is_secondary_output;
>>  	bool is_interlaced = params->dec.is_interlaced;
>> @@ -1235,7 +1236,12 @@ static int bufreq_dec(struct hfi_plat_buffers_params *params, u32 buftype,
>>  		bufreq->count_min = out_min_count;
>>  		bufreq->size =
>>  			venus_helper_get_framesz_raw(params->hfi_color_fmt,
>> -						     width, height);
>> +						     out_width, out_height);
>> +		if (buftype == HFI_BUFFER_OUTPUT &&
>> +		    params->dec.is_secondary_output)
I suppose this line could be unbroken

Konrad
>> +			bufreq->size =
>> +				venus_helper_get_framesz_raw(params->hfi_dpb_color_fmt,
>> +							     out_width, out_height);
>>  	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH(version)) {
>>  		bufreq->size = dec_ops->scratch(width, height, is_interlaced);
>>  	} else if (buftype == HFI_BUFFER_INTERNAL_SCRATCH_1(version)) {

[PATCH 3/4] venus: add handling of bit depth change from firmwar

[Dikshita Agarwal]

Set opb format to TP10_UWC and dpb to client set format
when bit depth change to 10 bit is detecting by firmware.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/venus/helpers.c | 18 ++++++++++++++++++
 drivers/media/platform/qcom/venus/vdec.c    |  5 ++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 4ad6232..4f48ebd 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1770,6 +1770,24 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 v4l2_fmt,
 	if (!caps)
 		return -EINVAL;
 
+	if (inst->bit_depth == VIDC_BITDEPTH_10 &&
+	    inst->session_type == VIDC_SESSION_TYPE_DEC) {
+		found_ubwc = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT,
+						HFI_COLOR_FORMAT_YUV420_TP10_UBWC);
+		found = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT2,
+					   fmt);
+		if (found_ubwc && found) {
+			/*
+			 * Hard-code DPB buffers to be 10bit UBWC
+			 * until V4L2 is able to expose compressed/tiled
+			 * formats to applications.
+			 */
+			*out_fmt = HFI_COLOR_FORMAT_YUV420_TP10_UBWC;
+			*out2_fmt = fmt;
+			return 0;
+		}
+	}
+
 	if (ubwc) {
 		ubwc_fmt = fmt | HFI_COLOR_FORMAT_UBWC_BASE;
 		found_ubwc = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT,
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 687d62e..69f7f6e 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -701,6 +701,9 @@ static int vdec_set_work_route(struct venus_inst *inst)
 }
 
 #define is_ubwc_fmt(fmt) (!!((fmt) & HFI_COLOR_FORMAT_UBWC_BASE))
+#define is_10bit_ubwc_fmt(fmt) (!!((fmt) & HFI_COLOR_FORMAT_10_BIT_BASE & \
+				 HFI_COLOR_FORMAT_UBWC_BASE))
+
 
 static int vdec_output_conf(struct venus_inst *inst)
 {
@@ -748,7 +751,7 @@ static int vdec_output_conf(struct venus_inst *inst)
 		inst->opb_fmt = out2_fmt;
 		inst->dpb_buftype = HFI_BUFFER_OUTPUT;
 		inst->dpb_fmt = out_fmt;
-	} else if (is_ubwc_fmt(out2_fmt)) {
+	} else if (is_ubwc_fmt(out2_fmt) || is_10bit_ubwc_fmt(out_fmt)) {
 		inst->opb_buftype = HFI_BUFFER_OUTPUT;
 		inst->opb_fmt = out_fmt;
 		inst->dpb_buftype = HFI_BUFFER_OUTPUT2;
-- 
2.7.4

Re: [PATCH 3/4] venus: add handling of bit depth change from firmwar

[Konrad Dybcio]

On 4.05.2023 12:36, Dikshita Agarwal wrote:
> Set opb format to TP10_UWC and dpb to client set format
> when bit depth change to 10 bit is detecting by firmware.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 18 ++++++++++++++++++
>  drivers/media/platform/qcom/venus/vdec.c    |  5 ++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 4ad6232..4f48ebd 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1770,6 +1770,24 @@ int venus_helper_get_out_fmts(struct venus_inst *inst, u32 v4l2_fmt,
>  	if (!caps)
>  		return -EINVAL;
>  
> +	if (inst->bit_depth == VIDC_BITDEPTH_10 &&
> +	    inst->session_type == VIDC_SESSION_TYPE_DEC) {
This could be unbroken

> +		found_ubwc = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT,
> +						HFI_COLOR_FORMAT_YUV420_TP10_UBWC);
> +		found = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT2,
> +					   fmt);
This could be unbroken

Otherwise I think this lgtm

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> +		if (found_ubwc && found) {
> +			/*
> +			 * Hard-code DPB buffers to be 10bit UBWC
> +			 * until V4L2 is able to expose compressed/tiled
> +			 * formats to applications.
> +			 */
> +			*out_fmt = HFI_COLOR_FORMAT_YUV420_TP10_UBWC;
> +			*out2_fmt = fmt;
> +			return 0;
> +		}
> +	}
> +
>  	if (ubwc) {
>  		ubwc_fmt = fmt | HFI_COLOR_FORMAT_UBWC_BASE;
>  		found_ubwc = find_fmt_from_caps(caps, HFI_BUFFER_OUTPUT,
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 687d62e..69f7f6e 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -701,6 +701,9 @@ static int vdec_set_work_route(struct venus_inst *inst)
>  }
>  
>  #define is_ubwc_fmt(fmt) (!!((fmt) & HFI_COLOR_FORMAT_UBWC_BASE))
> +#define is_10bit_ubwc_fmt(fmt) (!!((fmt) & HFI_COLOR_FORMAT_10_BIT_BASE & \
> +				 HFI_COLOR_FORMAT_UBWC_BASE))
> +
>  
>  static int vdec_output_conf(struct venus_inst *inst)
>  {
> @@ -748,7 +751,7 @@ static int vdec_output_conf(struct venus_inst *inst)
>  		inst->opb_fmt = out2_fmt;
>  		inst->dpb_buftype = HFI_BUFFER_OUTPUT;
>  		inst->dpb_fmt = out_fmt;
> -	} else if (is_ubwc_fmt(out2_fmt)) {
> +	} else if (is_ubwc_fmt(out2_fmt) || is_10bit_ubwc_fmt(out_fmt)) {
>  		inst->opb_buftype = HFI_BUFFER_OUTPUT;
>  		inst->opb_fmt = out_fmt;
>  		inst->dpb_buftype = HFI_BUFFER_OUTPUT2;

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

[Dikshita Agarwal]

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];
+	}
 
 	if (inst->pic_struct != ev_data->pic_struct)
 		inst->pic_struct = ev_data->pic_struct;
-- 
2.7.4

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

[Konrad Dybcio]

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;

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

[Dikshita Agarwal]

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;

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

[Konrad Dybcio]

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;

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

[Dmitry Baryshkov]

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;

-- 
With best wishes
Dmitry

Re: [PATCH 0/4] venus: add support for 10 bit decoding

[Bryan O'Donoghue]

On 04/05/2023 11:36, Dikshita Agarwal wrote:
> This series includes the changes to
>    - add V4L2_PIX_FMT_P010 as supported decoder format.
>    - consider dpb color format while calculating buffer
>      size for dpb buffers.
>    - update dpb and opb color format when bit depth
>      changes is detected, also update preferred color
>      format to P010 in this case.
> 
> With this series, divided the previous version [1] into
> multiple patches as suggested in review comments.
> 
> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=10376
> 
> Dikshita Agarwal (4):
>    venus: add support for V4L2_PIX_FMT_P010 color format
>    venus: update calculation for dpb buffers
>    venus: add handling of bit depth change from firmwar
>    venus: return P010 as preferred format for 10 bit decode
> 
>   drivers/media/platform/qcom/venus/helpers.c        | 24 ++++++++++++++++++++++
>   drivers/media/platform/qcom/venus/hfi_plat_bufs.h  |  3 +++
>   .../media/platform/qcom/venus/hfi_plat_bufs_v6.c   |  8 +++++++-
>   drivers/media/platform/qcom/venus/vdec.c           | 16 +++++++++++++--
>   4 files changed, 48 insertions(+), 3 deletions(-)
> 

For future reference a series like this should:

1. Include a log of what changed between the last series and this
2. Describe which comments you addressed
    I generally try to say
    "Added newline to dts - Konrad"
    "Sent the series as a -v3 - Bryan"
    etc
3. Ideally provide a link to the previous series in
 
https://lore.kernel.org/linux-arm-msm/1682492417-20496-1-git-send-email-quic_dikshita@quicinc.com/
4. Use versioning
    This set should be prefixed with "v2-0000-cover-letter" 
"v2-0001-add-support" etc

"git format-patch mybase..targettip --cover-letter -v2"

---
bod

Re: [PATCH 0/4] venus: add support for 10 bit decoding

[Bryan O'Donoghue]

On 04/05/2023 14:49, Bryan O'Donoghue wrote:
> On 04/05/2023 11:36, Dikshita Agarwal wrote:
>> This series includes the changes to
>>    - add V4L2_PIX_FMT_P010 as supported decoder format.
>>    - consider dpb color format while calculating buffer
>>      size for dpb buffers.
>>    - update dpb and opb color format when bit depth
>>      changes is detected, also update preferred color
>>      format to P010 in this case.
>>
>> With this series, divided the previous version [1] into
>> multiple patches as suggested in review comments.
>>
>> [1] https://patchwork.linuxtv.org/project/linux-media/list/?series=10376
>>
>> Dikshita Agarwal (4):
>>    venus: add support for V4L2_PIX_FMT_P010 color format
>>    venus: update calculation for dpb buffers
>>    venus: add handling of bit depth change from firmwar
>>    venus: return P010 as preferred format for 10 bit decode
>>
>>   drivers/media/platform/qcom/venus/helpers.c        | 24 
>> ++++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/hfi_plat_bufs.h  |  3 +++
>>   .../media/platform/qcom/venus/hfi_plat_bufs_v6.c   |  8 +++++++-
>>   drivers/media/platform/qcom/venus/vdec.c           | 16 +++++++++++++--
>>   4 files changed, 48 insertions(+), 3 deletions(-)
>>
> 
> For future reference a series like this should:
> 
> 1. Include a log of what changed between the last series and this
> 2. Describe which comments you addressed
>     I generally try to say
>     "Added newline to dts - Konrad"
>     "Sent the series as a -v3 - Bryan"
>     etc
> 3. Ideally provide a link to the previous series in
> 
> https://lore.kernel.org/linux-arm-msm/1682492417-20496-1-git-send-email-quic_dikshita@quicinc.com/
> 4. Use versioning
>     This set should be prefixed with "v2-0000-cover-letter" 
> "v2-0001-add-support" etc
> 
> "git format-patch mybase..targettip --cover-letter -v2"
> 
> ---
> bod

Doh I see you did most of that - just missed the V2.

Please remember to version your subsequent series. "git format-patch -v2"

---
bod

Re: [PATCH 0/4] venus: add support for 10 bit decoding

[Vikash Garodia]

On 5/4/2023 7:21 PM, Bryan O'Donoghue wrote:
> On 04/05/2023 14:49, Bryan O'Donoghue wrote:
>> On 04/05/2023 11:36, Dikshita Agarwal wrote:
>>> This series includes the changes to
>>>    - add V4L2_PIX_FMT_P010 as supported decoder format.
>>>    - consider dpb color format while calculating buffer
>>>      size for dpb buffers.
>>>    - update dpb and opb color format when bit depth
>>>      changes is detected, also update preferred color
>>>      format to P010 in this case.
>>>
>>> With this series, divided the previous version [1] into
>>> multiple patches as suggested in review comments.
>>>
>>> [1] 
>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=10376
>>>
>>> Dikshita Agarwal (4):
>>>    venus: add support for V4L2_PIX_FMT_P010 color format
>>>    venus: update calculation for dpb buffers
>>>    venus: add handling of bit depth change from firmwar
>>>    venus: return P010 as preferred format for 10 bit decode
>>>
>>>   drivers/media/platform/qcom/venus/helpers.c        | 24 
>>> ++++++++++++++++++++++
>>>   drivers/media/platform/qcom/venus/hfi_plat_bufs.h  |  3 +++
>>>   .../media/platform/qcom/venus/hfi_plat_bufs_v6.c   |  8 +++++++-
>>>   drivers/media/platform/qcom/venus/vdec.c           | 16 
>>> +++++++++++++--
>>>   4 files changed, 48 insertions(+), 3 deletions(-)
>>>
>>
>> For future reference a series like this should:
>>
>> 1. Include a log of what changed between the last series and this
>> 2. Describe which comments you addressed
>>     I generally try to say
>>     "Added newline to dts - Konrad"
>>     "Sent the series as a -v3 - Bryan"
>>     etc
>> 3. Ideally provide a link to the previous series in
>>
>> https://lore.kernel.org/linux-arm-msm/1682492417-20496-1-git-send-email-quic_dikshita@quicinc.com/ 
>>
>> 4. Use versioning
>>     This set should be prefixed with "v2-0000-cover-letter" 
>> "v2-0001-add-support" etc
>>
>> "git format-patch mybase..targettip --cover-letter -v2"
>>
>> ---
>> bod
>
> Doh I see you did most of that - just missed the V2.
>
> Please remember to version your subsequent series. "git format-patch -v2"

Does this qualify for a version upgrade when a single patch is 
subsequently raised as series ? IMO, the link

to previous single patch in cover letter and then starting the series 
(as v0) seems to provide the required info.

>
> ---
> bod

Re: [PATCH 0/4] venus: add support for 10 bit decoding

[Bryan O'Donoghue]

On 04/05/2023 15:04, Vikash Garodia wrote:
>> Doh I see you did most of that - just missed the V2.
>>
>> Please remember to version your subsequent series. "git format-patch -v2"
> 
> Does this qualify for a version upgrade when a single patch is 
> subsequently raised as series ? IMO, the link
> 
> to previous single patch in cover letter and then starting the series 
> (as v0) seems to provide the required info.

Hmm. I'd say any series should have an increment in it to differentiate, 
with the exception being RESEND.

Also you are splitting one patch into four.

Looking through a bunch of email it might be not immediately obvious to 
understand that the new series and old series differ, which is IMO how 
the version numbers help others to know what's going on.

---
bod