Re: [PATCH v4 13/14] media: ipu6: Drop custom functions to obtain sd state information

[Bingbu Cao <bingbu.cao@linux.intel.com>]

Sakari,

Thanks for the patch.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>

On 1/8/26 4:17 PM, Sakari Ailus wrote:
> Drop the custom functions that are used to obtain information from the
> sub-device state.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-isys-queue.c    | 17 ++++++---
>  .../media/pci/intel/ipu6/ipu6-isys-subdev.c   | 36 -------------------
>  .../media/pci/intel/ipu6/ipu6-isys-subdev.h   |  4 ---
>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 33 +++++++----------
>  4 files changed, 24 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> index 651ddab9ef14..c862de31af9c 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> @@ -420,7 +420,7 @@ static void buf_queue(struct vb2_buffer *vb)
>  
>  static int ipu6_isys_link_fmt_validate(struct ipu6_isys_queue *aq)
>  {
> -	struct v4l2_mbus_framefmt format;
> +	struct v4l2_mbus_framefmt format, *__format;
>  	struct ipu6_isys_video *av = ipu6_isys_queue_to_video(aq);
>  	struct device *dev = &av->isys->adev->auxdev.dev;
>  	struct media_pad *remote_pad =
> @@ -435,13 +435,20 @@ static int ipu6_isys_link_fmt_validate(struct ipu6_isys_queue *aq)
>  	sd = media_entity_to_v4l2_subdev(remote_pad->entity);
>  	r_stream = ipu6_isys_get_src_stream_by_src_pad(sd, remote_pad->index);
>  
> -	ret = ipu6_isys_get_stream_pad_fmt(sd, remote_pad->index, r_stream,
> -					   &format);
> +	struct v4l2_subdev_state *state =
> +		v4l2_subdev_lock_and_get_active_state(sd);
>  
> -	if (ret) {
> +	__format = v4l2_subdev_state_get_format(state, remote_pad->index,
> +						r_stream);
> +	if (__format)
> +		format = *__format;
> +
> +	v4l2_subdev_unlock_state(state);
> +
> +	if (!__format) {
>  		dev_dbg(dev, "failed to get %s: pad %d, stream:%d format\n",
>  			sd->entity.name, remote_pad->index, r_stream);
> -		return ret;
> +		return -EPIPE;
>  	}
>  
>  	if (format.width != ipu6_isys_get_frame_width(av) ||
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
> index 869e7d4ba572..dbd6f76a066d 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.c
> @@ -265,42 +265,6 @@ static int subdev_set_routing(struct v4l2_subdev *sd,
>  	return v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
>  }
>  
> -int ipu6_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
> -				 struct v4l2_mbus_framefmt *format)
> -{
> -	struct v4l2_mbus_framefmt *fmt;
> -	struct v4l2_subdev_state *state;
> -
> -	if (!sd || !format)
> -		return -EINVAL;
> -
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> -	fmt = v4l2_subdev_state_get_format(state, pad, stream);
> -	if (fmt)
> -		*format = *fmt;
> -	v4l2_subdev_unlock_state(state);
> -
> -	return fmt ? 0 : -EINVAL;
> -}
> -
> -int ipu6_isys_get_stream_pad_crop(struct v4l2_subdev *sd, u32 pad, u32 stream,
> -				  struct v4l2_rect *crop)
> -{
> -	struct v4l2_subdev_state *state;
> -	struct v4l2_rect *rect;
> -
> -	if (!sd || !crop)
> -		return -EINVAL;
> -
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> -	rect = v4l2_subdev_state_get_crop(state, pad, stream);
> -	if (rect)
> -		*crop = *rect;
> -	v4l2_subdev_unlock_state(state);
> -
> -	return rect ? 0 : -EINVAL;
> -}
> -
>  u32 ipu6_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad)
>  {
>  	struct v4l2_subdev_state *state;
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
> index 268dfa01e903..35069099c364 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-subdev.h
> @@ -38,10 +38,6 @@ int ipu6_isys_subdev_enum_mbus_code(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_mbus_code_enum
>  				    *code);
>  u32 ipu6_isys_get_src_stream_by_src_pad(struct v4l2_subdev *sd, u32 pad);
> -int ipu6_isys_get_stream_pad_fmt(struct v4l2_subdev *sd, u32 pad, u32 stream,
> -				 struct v4l2_mbus_framefmt *format);
> -int ipu6_isys_get_stream_pad_crop(struct v4l2_subdev *sd, u32 pad, u32 stream,
> -				  struct v4l2_rect *crop);
>  int ipu6_isys_subdev_set_routing(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *state,
>  				 enum v4l2_subdev_format_whence which,
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> index b035c7342a91..e9dab8a709ef 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> @@ -455,6 +455,7 @@ static int ipu6_isys_fw_pin_cfg(struct ipu6_isys_video *av,
>  {
>  	struct media_pad *src_pad = media_pad_remote_pad_first(&av->pad);
>  	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(src_pad->entity);
> +	struct v4l2_subdev_state *state = v4l2_subdev_get_locked_active_state(sd);
>  	struct ipu6_fw_isys_input_pin_info_abi *input_pin;
>  	struct ipu6_fw_isys_output_pin_info_abi *output_pin;
>  	struct ipu6_isys_stream *stream = av->stream;
> @@ -464,26 +465,13 @@ static int ipu6_isys_fw_pin_cfg(struct ipu6_isys_video *av,
>  		ipu6_isys_get_isys_format(ipu6_isys_get_format(av), 0);
>  	struct v4l2_rect v4l2_crop;
>  	struct ipu6_isys *isys = av->isys;
> -	struct device *dev = &isys->adev->auxdev.dev;
>  	int input_pins = cfg->nof_input_pins++;
>  	int output_pins;
>  	u32 src_stream;
> -	int ret;
>  
>  	src_stream = ipu6_isys_get_src_stream_by_src_pad(sd, src_pad->index);
> -	ret = ipu6_isys_get_stream_pad_fmt(sd, src_pad->index, src_stream,
> -					   &fmt);
> -	if (ret < 0) {
> -		dev_err(dev, "can't get stream format (%d)\n", ret);
> -		return ret;
> -	}
> -
> -	ret = ipu6_isys_get_stream_pad_crop(sd, src_pad->index, src_stream,
> -					    &v4l2_crop);
> -	if (ret < 0) {
> -		dev_err(dev, "can't get stream crop (%d)\n", ret);
> -		return ret;
> -	}
> +	fmt = *v4l2_subdev_state_get_format(state, src_pad->index, src_stream);
> +	v4l2_crop = *v4l2_subdev_state_get_crop(state, src_pad->index, src_stream);
>  
>  	input_pin = &cfg->input_pins[input_pins];
>  	input_pin->input_res.width = fmt.width;
> @@ -784,13 +772,16 @@ void ipu6_isys_configure_stream_watermark(struct ipu6_isys_video *av,
>  	csi2 = ipu6_isys_subdev_to_csi2(av->stream->asd);
>  	link_freq = ipu6_isys_csi2_get_link_freq(csi2);
>  	if (link_freq > 0) {
> +		struct v4l2_subdev_state *state =
> +			v4l2_subdev_lock_and_get_active_state(&csi2->asd.sd);
> +
>  		lanes = csi2->nlanes;
> -		ret = ipu6_isys_get_stream_pad_fmt(&csi2->asd.sd, 0,
> -						   av->source_stream, &format);
> -		if (!ret) {
> -			bpp = ipu6_isys_mbus_code_to_bpp(format.code);
> -			pixel_rate = mul_u64_u32_div(link_freq, lanes * 2, bpp);
> -		}
> +		format = *v4l2_subdev_state_get_format(state, 0,
> +						       av->source_stream);
> +		bpp = ipu6_isys_mbus_code_to_bpp(format.code);
> +		pixel_rate = mul_u64_u32_div(link_freq, lanes * 2, bpp);
> +
> +		v4l2_subdev_unlock_state(state);
>  	}
>  
>  	av->watermark.pixel_rate = pixel_rate;
> 

-- 
Best regards,
Bingbu Cao