Re: [PATCH 02/33] iris: vidc: add core functions

[Dikshita Agarwal]

On 8/1/2023 2:53 AM, Krzysztof Kozlowski wrote:
> On 28/07/2023 15:23, Vikash Garodia wrote:
>> From: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
>>
>> This implements the platform driver methods, file
>> operations and v4l2 registration.
> 
> 
>> +
>> +	core->debugfs_parent = msm_vidc_devm_debugfs_get(&pdev->dev);
>> +	if (!core->debugfs_parent)
>> +		d_vpr_h("Failed to create debugfs for msm_vidc\n");
>> +
>> +	rc = msm_vidc_devm_init_core(&pdev->dev, core);
>> +	if (rc) {
>> +		d_vpr_e("%s: init core failed with %d\n", __func__, rc);
>> +		goto init_core_failed;
>> +	}
>> +
>> +	rc = msm_vidc_init_platform(core);
>> +	if (rc) {
>> +		d_vpr_e("%s: init platform failed with %d\n", __func__, rc);
>> +		rc = -EINVAL;
>> +		goto init_plat_failed;
>> +	}
>> +
>> +	rc = msm_vidc_init_resources(core);
>> +	if (rc) {
>> +		d_vpr_e("%s: init resource failed with %d\n", __func__, rc);
>> +		goto init_res_failed;
> 
> NAK.
> 
> return dev_err_probe.
Sure, will update in next version.
> 
>> +	}
>> +
>> +	rc = msm_vidc_init_core_caps(core);
>> +	if (rc) {
>> +		d_vpr_e("%s: init core caps failed with %d\n", __func__, rc);
>> +		goto init_res_failed;
>> +	}
>> +
>> +	rc = msm_vidc_init_instance_caps(core);
>> +	if (rc) {
>> +		d_vpr_e("%s: init inst cap failed with %d\n", __func__, rc);
>> +		goto init_inst_caps_fail;
>> +	}
>> +
>> +	core->debugfs_root = msm_vidc_debugfs_init_core(core);
>> +	if (!core->debugfs_root)
>> +		d_vpr_h("Failed to init debugfs core\n");
>> +
>> +	d_vpr_h("populating sub devices\n");
>> +	/*
>> +	 * Trigger probe for each sub-device i.e. qcom,msm-vidc,context-bank.
>> +	 * When msm_vidc_probe is called for each sub-device, parse the
>> +	 * context-bank details.
>> +	 */
>> +	rc = of_platform_populate(pdev->dev.of_node, msm_vidc_dt_match, NULL,
>> +				  &pdev->dev);
>> +	if (rc) {
>> +		d_vpr_e("Failed to trigger probe for sub-devices\n");
>> +		goto sub_dev_failed;
>> +	}
>> +
>> +	rc = v4l2_device_register(&pdev->dev, &core->v4l2_dev);
>> +	if (rc) {
>> +		d_vpr_e("Failed to register v4l2 device\n");
>> +		goto v4l2_reg_failed;
>> +	}
>> +
>> +	/* setup the decoder device */
>> +	rc = msm_vidc_register_video_device(core, MSM_VIDC_DECODER, nr);
>> +	if (rc) {
>> +		d_vpr_e("Failed to register video decoder\n");
>> +		goto dec_reg_failed;
>> +	}
>> +
>> +	/* setup the encoder device */
>> +	rc = msm_vidc_register_video_device(core, MSM_VIDC_ENCODER, nr + 1);
>> +	if (rc) {
>> +		d_vpr_e("Failed to register video encoder\n");
>> +		goto enc_reg_failed;
>> +	}
>> +
>> +	rc = venus_hfi_queue_init(core);
>> +	if (rc) {
>> +		d_vpr_e("%s: interface queues init failed\n", __func__);
>> +		goto queues_init_failed;
>> +	}
>> +
>> +	rc = msm_vidc_core_init(core);
>> +	if (rc) {
>> +		d_vpr_e("%s: sys init failed\n", __func__);
>> +		goto core_init_failed;
>> +	}
>> +
>> +	d_vpr_h("%s(): succssful\n", __func__);
> 
> No need, drop.
Sure, will drop.
> 
>> +
>> +	return rc;
>> +
>> +core_init_failed:
>> +	venus_hfi_queue_deinit(core);
>> +queues_init_failed:
>> +	of_platform_depopulate(&pdev->dev);
>> +sub_dev_failed:
>> +	msm_vidc_unregister_video_device(core, MSM_VIDC_ENCODER);
>> +enc_reg_failed:
>> +	msm_vidc_unregister_video_device(core, MSM_VIDC_DECODER);
>> +dec_reg_failed:
>> +	v4l2_device_unregister(&core->v4l2_dev);
>> +v4l2_reg_failed:
>> +init_inst_caps_fail:
>> +init_res_failed:
>> +init_plat_failed:
>> +init_core_failed:
> 
> Ykes! No. This code needs more work.
> 
I understand, will improve the design in next version.
> Best regards,
> Krzysztof
>