Re: [PATCH v3 16/27] media: rockchip: rga: check scaling factor

["Sven Püschel" <s.pueschel@pengutronix.de>]

Hi Nicolas,

On 3/20/26 6:57 PM, Nicolas Dufresne wrote:
> Le mardi 27 janvier 2026 à 15:39 +0100, Sven Püschel a écrit :
>> Check the scaling factor to avoid potential problems. This is relevant
>> for the upcoming RGA3 support, as it can hang when the scaling factor
>> is exceeded.
>>
>> There are two relevant scenarios that have to be considered to protect
>> against invalid scaling values:
>>
>> When the output or capture is already streaming, setting the format on
>> the other side should consider the max scaling factor and clamp it
>> accordingly. This is only done in the streaming case, as it otherwise
>> may unintentionally clamp the value when the application sets the first
>> format (due to a default format on the other side).
>>
>> When the format is set on both sides first, then the format won't be
>> corrected by above means. Therefore the second streamon call has to
>> check the scaling factor and fail otherwise.
>>
>> As try functions should only be state aware if specified, the scaling
>> limitation is only done in s_fmt.
>>
>> Signed-off-by: Sven Püschel <s.pueschel@pengutronix.de>
>> ---
>>   drivers/media/platform/rockchip/rga/rga-hw.c |  1 +
>>   drivers/media/platform/rockchip/rga/rga-hw.h |  1 +
>>   drivers/media/platform/rockchip/rga/rga.c    | 47 ++++++++++++++++++++++++++++
>>   drivers/media/platform/rockchip/rga/rga.h    |  1 +
>>   4 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.c b/drivers/media/platform/rockchip/rga/rga-hw.c
>> index dcd540ed3fd5b..7a4070665fed7 100644
>> --- a/drivers/media/platform/rockchip/rga/rga-hw.c
>> +++ b/drivers/media/platform/rockchip/rga/rga-hw.c
>> @@ -584,6 +584,7 @@ const struct rga_hw rga2_hw = {
>>   	.max_width = MAX_WIDTH,
>>   	.min_height = MIN_HEIGHT,
>>   	.max_height = MAX_HEIGHT,
>> +	.max_scaling_factor = MAX_SCALING_FACTOR,
>>   	.stride_alignment = 4,
>>   
>>   	.setup_cmdbuf = rga_hw_setup_cmdbuf,
>> diff --git a/drivers/media/platform/rockchip/rga/rga-hw.h b/drivers/media/platform/rockchip/rga/rga-hw.h
>> index f4752aa823051..fffcab0131225 100644
>> --- a/drivers/media/platform/rockchip/rga/rga-hw.h
>> +++ b/drivers/media/platform/rockchip/rga/rga-hw.h
>> @@ -14,6 +14,7 @@
>>   
>>   #define MIN_WIDTH 34
>>   #define MIN_HEIGHT 34
>> +#define MAX_SCALING_FACTOR 16
>>   
>>   #define RGA_TIMEOUT 500
>>   
>> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
>> index 6947c472a8b01..fad921ddd8348 100644
>> --- a/drivers/media/platform/rockchip/rga/rga.c
>> +++ b/drivers/media/platform/rockchip/rga/rga.c
>> @@ -405,10 +405,36 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
>>   	struct v4l2_pix_format_mplane *pix_fmt = &f->fmt.pix_mp;
>>   	struct rga_ctx *ctx = file_to_rga_ctx(file);
>>   	struct rockchip_rga *rga = ctx->rga;
>> +	const struct rga_hw *hw = rga->hw;
>>   	struct vb2_queue *vq;
>>   	struct rga_frame *frm;
>>   	int ret = 0;
>>   	int i;
>> +	struct rga_frame *limit_frm = NULL;
>> +
>> +	/* Limit before try_fmt to avoid recalculating the stride */
>> +	if (V4L2_TYPE_IS_OUTPUT(f->type) &&
>> +	    v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx)->streaming)
>> +		limit_frm = &ctx->out;
>> +	if (V4L2_TYPE_IS_CAPTURE(f->type) &&
>> +	    v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx)->streaming)
>> +		limit_frm = &ctx->in;
>> +	if (limit_frm) {
>> +		const struct v4l2_frmsize_stepwise frmsize = {
>> +			.min_width = DIV_ROUND_UP(limit_frm->pix.width,
>> +						  hw->max_scaling_factor),
>> +			.max_width =
>> +				limit_frm->pix.width * hw->max_scaling_factor,
>> +			.min_height = DIV_ROUND_UP(limit_frm->pix.height,
>> +						   hw->max_scaling_factor),
>> +			.max_height =
>> +				limit_frm->pix.height * hw->max_scaling_factor,
>> +			.step_width = 1,
>> +			.step_height = 1,
> Is there a risk to re-introduce odd sizes ? Should this be the hdiv / vdiv
> values ?

I'm still calling try_fmt afterwards, which does all of the further 
clamping (min/max size supported by the core and the yuv step size). 
Therefore I don't see the risk of re-introducing invalid sizes. It's 
just an additional clamping only done in s_fmt to avoid the try_fmt 
getting stateful.

Sincerely
     Sven

>
>> +		};
>> +		v4l2_apply_frmsize_constraints(&pix_fmt->width,
>> +					       &pix_fmt->height, &frmsize);
>> +	}
>>   
>>   	/* Adjust all values accordingly to the hardware capabilities
>>   	 * and chosen format.
>> @@ -568,12 +594,33 @@ static int vidioc_s_selection(struct file *file, void *priv,
>>   	return ret;
>>   }
>>   
>> +static bool check_scaling(const struct rga_hw *hw, u32 src_size, u32 dst_size)
>> +{
>> +	if (src_size < dst_size)
>> +		return src_size * hw->max_scaling_factor >= dst_size;
>> +	else
>> +		return dst_size * hw->max_scaling_factor >= src_size;
>> +}
>> +
>>   static int vidioc_streamon(struct file *file, void *priv,
>>   			   enum v4l2_buf_type type)
>>   {
>>   	struct rga_ctx *ctx = file_to_rga_ctx(file);
>>   	const struct rga_hw *hw = ctx->rga->hw;
>>   
>> +	if ((V4L2_TYPE_IS_OUTPUT(type) &&
>> +	     v4l2_m2m_get_dst_vq(ctx->fh.m2m_ctx)->streaming) ||
>> +	    (V4L2_TYPE_IS_CAPTURE(type) &&
>> +	     v4l2_m2m_get_src_vq(ctx->fh.m2m_ctx)->streaming)) {
>> +		/*
>> +		 * As the other side is already streaming,
>> +		 * check that the max scaling factor isn't exceeded.
>> +		 */
>> +		if (!check_scaling(hw, ctx->in.pix.width, ctx->out.pix.width) ||
>> +		    !check_scaling(hw, ctx->in.pix.height, ctx->out.pix.height))
>> +			return -EINVAL;
>> +	}
>> +
>>   	hw->setup_cmdbuf(ctx);
>>   
>>   	return v4l2_m2m_streamon(file, ctx->fh.m2m_ctx, type);
>> diff --git a/drivers/media/platform/rockchip/rga/rga.h b/drivers/media/platform/rockchip/rga/rga.h
>> index c741213710b32..454af283b1694 100644
>> --- a/drivers/media/platform/rockchip/rga/rga.h
>> +++ b/drivers/media/platform/rockchip/rga/rga.h
>> @@ -150,6 +150,7 @@ struct rga_hw {
>>   	size_t cmdbuf_size;
>>   	u32 min_width, min_height;
>>   	u32 max_width, max_height;
>> +	u8 max_scaling_factor;
>>   	u8 stride_alignment;
>>   
>>   	void (*setup_cmdbuf)(struct rga_ctx *ctx);