Re: [PATCH v4 12/28] media: iris: implement enum_fmt and enum_frameintervals ioctls

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

On 10/23/2024 4:19 PM, Hans Verkuil wrote:
> On 14/10/2024 11:07, Dikshita Agarwal wrote:
>> From: Vedang Nagar <quic_vnagar@quicinc.com>
>>
>> Implement enum_fmt and enum_frameintervals ioctls with
>> necessary hooks.
>>
>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
>>  .../platform/qcom/iris/iris_platform_common.h      |  4 +++
>>  .../platform/qcom/iris/iris_platform_sm8550.c      |  4 +++
>>  drivers/media/platform/qcom/iris/iris_vdec.c       | 21 ++++++++++++
>>  drivers/media/platform/qcom/iris/iris_vdec.h       |  1 +
>>  drivers/media/platform/qcom/iris/iris_vidc.c       | 39 ++++++++++++++++++++++
>>  5 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
>> index e345667dfbf2..54a2d723797b 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
>> @@ -43,6 +43,10 @@ struct ubwc_config_data {
>>  };
>>  
>>  struct platform_inst_caps {
>> +	u32 min_frame_width;
>> +	u32 max_frame_width;
>> +	u32 min_frame_height;
>> +	u32 max_frame_height;
>>  	u32 max_mbpf;
>>  };
>>  struct iris_core_power {
>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> index bc4769732aad..37c0130d7059 100644
>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
>> @@ -11,6 +11,10 @@
>>  #define VIDEO_ARCH_LX 1
>>  
>>  static struct platform_inst_caps platform_inst_cap_sm8550 = {
>> +	.min_frame_width = 96,
>> +	.max_frame_width = 8192,
>> +	.min_frame_height = 96,
>> +	.max_frame_height = 8192,
>>  	.max_mbpf = (8192 * 4352) / 256,
>>  };
>>  
>> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c
>> index e807decdda2b..fd0f1ebc33e8 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vdec.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c
>> @@ -60,6 +60,27 @@ void iris_vdec_inst_deinit(struct iris_inst *inst)
>>  	kfree(inst->fmt_src);
>>  }
>>  
>> +int iris_vdec_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc *f)
>> +{
>> +	switch (f->type) {
>> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> +		f->pixelformat = V4L2_PIX_FMT_H264;
>> +		f->flags = V4L2_FMT_FLAG_COMPRESSED | V4L2_FMT_FLAG_DYN_RESOLUTION;
>> +		strscpy(f->description, "codec", sizeof(f->description));
> 
> Don't set description, it's handled in v4l_fill_fmtdesc in v4l2-ioctl.c.
> 
>> +		break;
>> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>> +		f->pixelformat = V4L2_PIX_FMT_NV12;
>> +		strscpy(f->description, "colorformat", sizeof(f->description));
> 
> Ditto.
> 
> Hmm, v4l2-compliance should warn about this. Is this changed in a later patch perhaps?
> 
Oh, we didn't see such warning.
Will make the change.
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	memset(f->reserved, 0, sizeof(f->reserved));
> 
> No need to do this, it's already zeroed by v4l_enum_fmt.
> 
Sure, Noted.
>> +
>> +	return 0;
>> +}
>> +
>>  int iris_vdec_try_fmt(struct iris_inst *inst, struct v4l2_format *f)
>>  {
>>  	struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp;
>> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.h b/drivers/media/platform/qcom/iris/iris_vdec.h
>> index 4f2557d15ca2..eb8a1121ae92 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vdec.h
>> +++ b/drivers/media/platform/qcom/iris/iris_vdec.h
>> @@ -10,6 +10,7 @@ struct iris_inst;
>>  
>>  void iris_vdec_inst_init(struct iris_inst *inst);
>>  void iris_vdec_inst_deinit(struct iris_inst *inst);
>> +int iris_vdec_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc *f);
>>  int iris_vdec_try_fmt(struct iris_inst *inst, struct v4l2_format *f);
>>  int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f);
>>  
>> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
>> index 481fa0a7b7f3..1d6c5e8fafb4 100644
>> --- a/drivers/media/platform/qcom/iris/iris_vidc.c
>> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
>> @@ -214,6 +214,16 @@ int iris_close(struct file *filp)
>>  	return 0;
>>  }
>>  
>> +static int iris_enum_fmt(struct file *filp, void *fh, struct v4l2_fmtdesc *f)
>> +{
>> +	struct iris_inst *inst = iris_get_inst(filp, NULL);
>> +
>> +	if (f->index)
>> +		return -EINVAL;
>> +
>> +	return iris_vdec_enum_fmt(inst, f);
>> +}
>> +
>>  static int iris_try_fmt_vid_mplane(struct file *filp, void *fh, struct v4l2_format *f)
>>  {
>>  	struct iris_inst *inst = iris_get_inst(filp, NULL);
>> @@ -256,6 +266,32 @@ static int iris_g_fmt_vid_mplane(struct file *filp, void *fh, struct v4l2_format
>>  	return ret;
>>  }
>>  
>> +static int iris_enum_framesizes(struct file *filp, void *fh,
>> +				struct v4l2_frmsizeenum *fsize)
>> +{
>> +	struct iris_inst *inst = iris_get_inst(filp, NULL);
>> +	struct platform_inst_caps *caps;
>> +
>> +	if (fsize->index)
>> +		return -EINVAL;
>> +
>> +	if (fsize->pixel_format != V4L2_PIX_FMT_H264 &&
>> +	    fsize->pixel_format != V4L2_PIX_FMT_NV12)
>> +		return -EINVAL;
>> +
>> +	caps = inst->core->iris_platform_data->inst_caps;
>> +
>> +	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
>> +	fsize->stepwise.min_width = caps->min_frame_width;
>> +	fsize->stepwise.max_width = caps->max_frame_width;
>> +	fsize->stepwise.step_width = STEP_WIDTH;
>> +	fsize->stepwise.min_height = caps->min_frame_height;
>> +	fsize->stepwise.max_height = caps->max_frame_height;
>> +	fsize->stepwise.step_height = STEP_HEIGHT;
>> +
>> +	return 0;
>> +}
>> +
>>  static int iris_g_selection(struct file *filp, void *fh, struct v4l2_selection *s)
>>  {
>>  	struct iris_inst *inst = iris_get_inst(filp, NULL);
>> @@ -298,12 +334,15 @@ static const struct vb2_ops iris_vb2_ops = {
>>  };
>>  
>>  static const struct v4l2_ioctl_ops iris_v4l2_ioctl_ops = {
>> +	.vidioc_enum_fmt_vid_cap        = iris_enum_fmt,
>> +	.vidioc_enum_fmt_vid_out        = iris_enum_fmt,
>>  	.vidioc_try_fmt_vid_cap_mplane  = iris_try_fmt_vid_mplane,
>>  	.vidioc_try_fmt_vid_out_mplane  = iris_try_fmt_vid_mplane,
>>  	.vidioc_s_fmt_vid_cap_mplane    = iris_s_fmt_vid_mplane,
>>  	.vidioc_s_fmt_vid_out_mplane    = iris_s_fmt_vid_mplane,
>>  	.vidioc_g_fmt_vid_cap_mplane    = iris_g_fmt_vid_mplane,
>>  	.vidioc_g_fmt_vid_out_mplane    = iris_g_fmt_vid_mplane,
>> +	.vidioc_enum_framesizes         = iris_enum_framesizes,
>>  	.vidioc_reqbufs                 = v4l2_m2m_ioctl_reqbufs,
>>  	.vidioc_g_selection             = iris_g_selection,
>>  };
>>
>