Re: [PATCH v3 16/29] media: iris: implement iris v4l2_ctrl_ops and prepare capabilities

[Dmitry Baryshkov]

On Tue, Oct 01, 2024 at 06:31:16PM GMT, Vedang Nagar wrote:
> Hi Dmitry,
> 
> On 8/29/2024 3:03 PM, Dmitry Baryshkov wrote:
> > On Tue, Aug 27, 2024 at 03:35:41PM GMT, Dikshita Agarwal via B4 Relay wrote:
> >> From: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
> >>
> >> Implement s_ctrl and g_volatile_ctrl ctrl ops.
> >> Introduce platform specific driver and firmware capabilities.
> >> Capabilities are set of video specifications
> >> and features supported by a specific platform (SOC).
> >> Each capability is defined with min, max, range, default
> >> value and corresponding HFI.
> >>
> >> Signed-off-by: Vedang Nagar <quic_vnagar@xxxxxxxxxxx>
> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> >> ---
> >>  drivers/media/platform/qcom/iris/Makefile          |   1 +
> >>  drivers/media/platform/qcom/iris/iris_buffer.c     |   3 +-
> >>  drivers/media/platform/qcom/iris/iris_core.h       |   2 +
> >>  drivers/media/platform/qcom/iris/iris_ctrls.c      | 194 +++++++++++++++++++++
> >>  drivers/media/platform/qcom/iris/iris_ctrls.h      |  15 ++
> >>  .../platform/qcom/iris/iris_hfi_gen1_defines.h     |   4 +
> >>  .../platform/qcom/iris/iris_hfi_gen2_command.c     |   1 +
> >>  .../platform/qcom/iris/iris_hfi_gen2_defines.h     |   9 +
> >>  drivers/media/platform/qcom/iris/iris_instance.h   |   6 +
> >>  .../platform/qcom/iris/iris_platform_common.h      |  71 ++++++++
> >>  .../platform/qcom/iris/iris_platform_sm8250.c      |  56 ++++++
> >>  .../platform/qcom/iris/iris_platform_sm8550.c      | 138 +++++++++++++++
> >>  drivers/media/platform/qcom/iris/iris_probe.c      |   7 +
> >>  drivers/media/platform/qcom/iris/iris_vdec.c       |  24 ++-
> >>  drivers/media/platform/qcom/iris/iris_vdec.h       |   2 +-
> >>  drivers/media/platform/qcom/iris/iris_vidc.c       |  16 +-
> >>  16 files changed, 536 insertions(+), 13 deletions(-)
> 
> [Skipped]
> 
> >> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> >> index a74114b0761a..6ad2ca7be0f0 100644
> >> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> >> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> >> @@ -108,6 +108,7 @@ static int iris_hfi_gen2_session_set_default_header(struct iris_inst *inst)
> >>  	struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
> >>  	u32 default_header = false;
> >>  
> >> +	default_header = inst->fw_cap[DEFAULT_HEADER].value;
> > 
> > This isn't related to the s_ctrl and g_volatile_ctrl. Please split this
> > commit into the chunk that is actually related to that API and the rest
> > of the changes.
> Could you please help to provide more details on how are you expecting the
> split of the patches?
> 
> Do you expect to split V4L2 ctrls_init/s_ctrl/g_ctrl in one patch and the
> introduction of all the capabilities into another patch? We are not finding
> it feasible to split the patch that way as in ctrl_init we read the
> capability from platform data to initialize the respective control.

Please split all caps and all the structs that are not related to the
V4L2 ctrls implementation. In this patch please keep only those defines,
structs and fields that are required to implement V4L2 ctrl API.

> > 
> >>  	iris_hfi_gen2_packet_session_property(inst,
> >>  					      HFI_PROP_DEC_DEFAULT_HEADER,
> >>  					      HFI_HOST_FLAGS_NONE,
> > 
> > 

-- 
With best wishes
Dmitry