Re: [PATCH 1/2] media: qcom: iris: venc: keep the crop rectangle in visible resolution space

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 12/11/2026 16:15, Loic Poulain wrote:
> The encoder keeps two source resolutions. inst->fmt_src holds the padded
> buffer geometry handed to userspace, with width aligned to 128 and height
> to 32. enc_raw_width and enc_raw_height hold the visible resolution sent
> to the firmware as HFI_PROP_RAW_RESOLUTION.
> 
> inst->crop must use the visible resolution, because
> iris_hfi_gen2_set_crop_offsets() computes
> 
> 	bottom_offset = inst->enc_raw_height - inst->crop.height;
> 
> iris_venc_s_fmt_input() seeded inst->crop from the padded fmt_src instead,
> so any height that is not a multiple of 32 underflows. 

I think this explains the bug enough TBH.

<snip>For 1280x720,
> crop.height becomes 736 and bottom_offset becomes 0xfffffff0. The two
> offsets share one payload word, so the right offset is corrupted too.
> 
> iris_venc_s_fmt_output() compared the requested coded size against
> fmt_src, so a plain 1280x720 session looked like a 736 -> 720 downscale.
> Seed fmt_dst from the visible size. The encoder does not use fmt_dst for
> stride programming.
> 
> Seeding fmt_dst from the visible size also means the codec alignment has
> to be applied on every path and not only when scaling, so that the
> CAPTURE queue keeps reporting the coded size. For 1920x1080 that is 1088,
> and the firmware signals the visible 1080 with SPS frame cropping.
</snip>

> Report CROP_BOUNDS and CROP_DEFAULT from enc_raw_*, so the bounds match
> the real source frame.

> G_FMT is left as is. For contiguous NV12 pix_mp.height is the only way to
> report the vertical luma padding, and the hardware is programmed with
> scanline_y = ALIGN(h, 32).

>    v4l2-ctl -d /dev/video1 \
>        --set-fmt-video-out=width=1280,height=720,pixelformat=NV12 \
>        --get-selection-output=target=crop
> 
> Before:
> 
>    Selection Video Output: crop, Left 0, Top 0, Width 1280, Height 736
> 
> After:
> 
>    Selection Video Output: crop, Left 0, Top 0, Width 1280, Height 720

Nice.

> Fixes: d9967fa37ca9 ("media: qcom: iris: Improve format alignment for encoder")
> Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> ---
>   drivers/media/platform/qcom/iris/iris_venc.c | 20 ++++++++++++--------
>   drivers/media/platform/qcom/iris/iris_vidc.c |  4 ++--
>   2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_venc.c b/drivers/media/platform/qcom/iris/iris_venc.c
> index 2f2c56bf9122c73e10e86815b1aa5fad99b0fb42..b367367ac2ccf874a00606b8b1352fed37b37a57 100644
> --- a/drivers/media/platform/qcom/iris/iris_venc.c
> +++ b/drivers/media/platform/qcom/iris/iris_venc.c
> @@ -240,14 +240,18 @@ static int iris_venc_s_fmt_output(struct iris_inst *inst, struct v4l2_format *f)
>   	 * If output format size != input format size,
>   	 * it is considered a scaling case,
>   	 * and the scaled size needs to be saved.
> +	 * Compare against the visible source resolution.
>   	 */
> -	if (f->fmt.pix_mp.width != inst->fmt_src->fmt.pix_mp.width ||
> -	    f->fmt.pix_mp.height != inst->fmt_src->fmt.pix_mp.height) {
> +	if (f->fmt.pix_mp.width != inst->enc_raw_width ||
> +	    f->fmt.pix_mp.height != inst->enc_raw_height) {
>   		inst->enc_scale_width = f->fmt.pix_mp.width;
>   		inst->enc_scale_height = f->fmt.pix_mp.height;
> -		fmt->fmt.pix_mp.width = ALIGN(f->fmt.pix_mp.width, codec_align);
> -		fmt->fmt.pix_mp.height = ALIGN(f->fmt.pix_mp.height, codec_align);
>   	}
> +
> +	/* The CAPTURE queue reports the coded size, in whole macroblocks. */
> +	fmt->fmt.pix_mp.width = ALIGN(f->fmt.pix_mp.width, codec_align);
> +	fmt->fmt.pix_mp.height = ALIGN(f->fmt.pix_mp.height, codec_align);
> +
>   	fmt->fmt.pix_mp.num_planes = 1;
>   	fmt->fmt.pix_mp.plane_fmt[0].bytesperline = 0;
>   	fmt->fmt.pix_mp.plane_fmt[0].sizeimage = iris_get_buffer_size(inst, BUF_OUTPUT);
> @@ -293,8 +297,8 @@ static int iris_venc_s_fmt_input(struct iris_inst *inst, struct v4l2_format *f)
>   	fmt->fmt.pix_mp.quantization = f->fmt.pix_mp.quantization;
>   
>   	output_fmt = inst->fmt_dst;
> -	output_fmt->fmt.pix_mp.width = fmt->fmt.pix_mp.width;
> -	output_fmt->fmt.pix_mp.height = fmt->fmt.pix_mp.height;
> +	output_fmt->fmt.pix_mp.width = f->fmt.pix_mp.width;
> +	output_fmt->fmt.pix_mp.height = f->fmt.pix_mp.height;
>   	output_fmt->fmt.pix_mp.colorspace = fmt->fmt.pix_mp.colorspace;
>   	output_fmt->fmt.pix_mp.xfer_func = fmt->fmt.pix_mp.xfer_func;
>   	output_fmt->fmt.pix_mp.ycbcr_enc = fmt->fmt.pix_mp.ycbcr_enc;
> @@ -312,8 +316,8 @@ static int iris_venc_s_fmt_input(struct iris_inst *inst, struct v4l2_format *f)
>   	    f->fmt.pix_mp.height != inst->crop.height) {
>   		inst->crop.top = 0;
>   		inst->crop.left = 0;
> -		inst->crop.width = fmt->fmt.pix_mp.width;
> -		inst->crop.height = fmt->fmt.pix_mp.height;
> +		inst->crop.width = f->fmt.pix_mp.width;
> +		inst->crop.height = f->fmt.pix_mp.height;
>   
>   		iris_venc_s_fmt_output(inst, output_fmt);
>   	}
> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> index 05aab6e3005d5c3788f5dfe5857f2d6b782e7a83..e3eaec1b7c2e30c065d8419ac3278756f70a8a97 100644
> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
> @@ -522,8 +522,8 @@ static int iris_g_selection(struct file *filp, void *fh, struct v4l2_selection *
>   		switch (s->target) {
>   		case V4L2_SEL_TGT_CROP_BOUNDS:
>   		case V4L2_SEL_TGT_CROP_DEFAULT:
> -			s->r.width = inst->fmt_src->fmt.pix_mp.width;
> -			s->r.height = inst->fmt_src->fmt.pix_mp.height;
> +			s->r.width = inst->enc_raw_width;
> +			s->r.height = inst->enc_raw_height;
>   			break;
>   		case V4L2_SEL_TGT_CROP:
>   			s->r.width = inst->crop.width;
> 

Could crop and scaling go into separate patches ? Feels like it could be 
made more granular.

---
bod