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

[Konrad Dybcio]

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.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> Signed-off-by: Vikash Garodia <quic_vgarodia@xxxxxxxxxxx>
> ---
[...]

> +struct msm_vidc_core *g_core;
> +
> +static inline bool is_video_device(struct device *dev)
> +{
> +	return !!(of_device_is_compatible(dev->of_node, "qcom,sm8550-vidc"));
> +}
Are you expecting this to be expanded each time support for new SoC is added?

> +
> +static inline bool is_video_context_bank_device(struct device *dev)
> +{
> +	return !!(of_device_is_compatible(dev->of_node, "qcom,vidc,cb-ns"));
> +}
> +
> +static int msm_vidc_init_resources(struct msm_vidc_core *core)
> +{
> +	struct msm_vidc_resource *res = NULL;
No need to initialize, you use it right after defining.

> +	int rc = 0;
I think 'ret' is more common for a return-value-holding variable.

> +
> +	res = devm_kzalloc(&core->pdev->dev, sizeof(*res), GFP_KERNEL);
> +	if (!res) {
> +		d_vpr_e("%s: failed to alloc memory for resource\n", __func__);
> +		return -ENOMEM;
> +	}
> +	core->resource = res;
I don't think the 'res' variable makes sense.

> +
> +	rc = call_res_op(core, init, core);
> +	if (rc) {
> +		d_vpr_e("%s: Failed to init resources: %d\n", __func__, rc);
> +		return rc;
you can omit this line and return rc/ret at the last line of this func.

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id msm_vidc_dt_match[] = {
> +	{.compatible = "qcom,sm8550-vidc"},
{ .compatible = .... " },

> +	{.compatible = "qcom,vidc,cb-ns"},
> +	MSM_VIDC_EMPTY_BRACE
why?

> +};
> +MODULE_DEVICE_TABLE(of, msm_vidc_dt_match);
> +
> +static void msm_vidc_release_video_device(struct video_device *vdev)
> +{
> +	d_vpr_e("%s: video device released\n", __func__);
> +}
Doesn't sound too useful? And definitely not with an error print?

> +
> +static void msm_vidc_unregister_video_device(struct msm_vidc_core *core,
> +					     enum msm_vidc_domain_type type)
> +{
> +	int index;
> +
> +	if (type == MSM_VIDC_DECODER)
I'm not sure this is defined.

> +		index = 0;
> +	else if (type == MSM_VIDC_ENCODER)
Or this.

Can't we just assign index = MSM_VIDC_EN/DECODER?

> +		index = 1;
> +	else
> +		return;
> +
> +	v4l2_m2m_release(core->vdev[index].m2m_dev);
> +
> +	video_set_drvdata(&core->vdev[index].vdev, NULL);
> +	video_unregister_device(&core->vdev[index].vdev);
> +}
> +
> +static int msm_vidc_register_video_device(struct msm_vidc_core *core,
> +					  enum msm_vidc_domain_type type, int nr)
> +{
> +	int rc = 0;
> +	int index;
> +
> +	d_vpr_h("%s: domain %d\n", __func__, type);
> +
> +	if (type == MSM_VIDC_DECODER)
> +		index = 0;
> +	else if (type == MSM_VIDC_ENCODER)
> +		index = 1;
> +	else
> +		return -EINVAL;
> +
> +	core->vdev[index].vdev.release =
> +		msm_vidc_release_video_device;
> +	core->vdev[index].vdev.fops = core->v4l2_file_ops;
> +	if (type == MSM_VIDC_DECODER)
> +		core->vdev[index].vdev.ioctl_ops = core->v4l2_ioctl_ops_dec;
> +	else
> +		core->vdev[index].vdev.ioctl_ops = core->v4l2_ioctl_ops_enc;
> +	core->vdev[index].vdev.vfl_dir = VFL_DIR_M2M;
> +	core->vdev[index].type = type;
> +	core->vdev[index].vdev.v4l2_dev = &core->v4l2_dev;
> +	core->vdev[index].vdev.device_caps = core->capabilities[DEVICE_CAPS].value;
> +	rc = video_register_device(&core->vdev[index].vdev,
> +				   VFL_TYPE_VIDEO, nr);
> +	if (rc) {
> +		d_vpr_e("Failed to register the video device\n");
> +		return rc;
> +	}
> +	video_set_drvdata(&core->vdev[index].vdev, core);
> +
> +	core->vdev[index].m2m_dev = v4l2_m2m_init(core->v4l2_m2m_ops);
> +	if (IS_ERR(core->vdev[index].m2m_dev)) {
> +		d_vpr_e("Failed to initialize V4L2 M2M device\n");
> +		rc = PTR_ERR(core->vdev[index].m2m_dev);
> +		goto m2m_init_failed;
> +	}
> +
> +	return 0;
> +
> +m2m_init_failed:
> +	video_unregister_device(&core->vdev[index].vdev);
> +	return rc;
> +}
> +
> +static int msm_vidc_deinitialize_core(struct msm_vidc_core *core)
> +{
> +	int rc = 0;
> +
> +	if (!core) {
Are we expecting to ever hit this?

> +		d_vpr_e("%s: invalid params\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	mutex_destroy(&core->lock);
> +	msm_vidc_update_core_state(core, MSM_VIDC_CORE_DEINIT, __func__);
Not defined.

> +
> +	if (core->batch_workq)
> +		destroy_workqueue(core->batch_workq);
> +
> +	if (core->pm_workq)
> +		destroy_workqueue(core->pm_workq);
> +
> +	core->batch_workq = NULL;
> +	core->pm_workq = NULL;
> +
> +	return rc;
> +}
> +
> +static int msm_vidc_initialize_core(struct msm_vidc_core *core)
> +{
> +	int rc = 0;
> +
> +	msm_vidc_update_core_state(core, MSM_VIDC_CORE_DEINIT, __func__);
Not defined.

> +
> +	core->pm_workq = create_singlethread_workqueue("pm_workq");
> +	if (!core->pm_workq) {
> +		d_vpr_e("%s: create pm workq failed\n", __func__);
> +		rc = -EINVAL;
> +		goto exit;
> +	}
> +
> +	core->batch_workq = create_singlethread_workqueue("batch_workq");
> +	if (!core->batch_workq) {
> +		d_vpr_e("%s: create batch workq failed\n", __func__);
> +		rc = -EINVAL;
> +		goto exit;
> +	}
> +
> +	core->packet_size = VIDC_IFACEQ_VAR_HUGE_PKT_SIZE;
> +	core->packet = devm_kzalloc(&core->pdev->dev, core->packet_size, GFP_KERNEL);
> +	if (!core->packet) {
> +		d_vpr_e("%s: failed to alloc core packet\n", __func__);
> +		rc = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	core->response_packet = devm_kzalloc(&core->pdev->dev, core->packet_size, GFP_KERNEL);
> +	if (!core->packet) {
> +		d_vpr_e("%s: failed to alloc core response packet\n", __func__);
> +		rc = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	mutex_init(&core->lock);
> +	INIT_LIST_HEAD(&core->instances);
> +	INIT_LIST_HEAD(&core->dangling_instances);
> +
> +	INIT_DELAYED_WORK(&core->pm_work, venus_hfi_pm_work_handler);
> +	INIT_DELAYED_WORK(&core->fw_unload_work, msm_vidc_fw_unload_handler);
> +
> +	return 0;
Either return rc/ret here or don't initialize it at definition.

> +exit:
> +	if (core->batch_workq)
> +		destroy_workqueue(core->batch_workq);
> +	if (core->pm_workq)
> +		destroy_workqueue(core->pm_workq);
> +	core->batch_workq = NULL;
> +	core->pm_workq = NULL;
> +
> +	return rc;
> +}
[...]

> +
> +static int msm_vidc_pm_suspend(struct device *dev)
> +{
> +	int rc = 0;
> +	struct msm_vidc_core *core;
> +	enum msm_vidc_allow allow = MSM_VIDC_DISALLOW;
> +
> +	/*
> +	 * Bail out if
> +	 * - driver possibly not probed yet
Would the pm callbacks be registered by then?

> +	 * - not the main device. We don't support power management on
> +	 *   subdevices (e.g. context banks)
I'm not sure registering context banks as different kinds of devices
within the same driver is a good idea, this seems rather convoluted.

> +	 */
> +	if (!dev || !dev->driver || !is_video_device(dev))
> +		return 0;
> +
> +	core = dev_get_drvdata(dev);
> +	if (!core) {
> +		d_vpr_e("%s: invalid core\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	core_lock(core, __func__);
> +	allow = msm_vidc_allow_pm_suspend(core);
> +
> +	if (allow == MSM_VIDC_IGNORE) {
> +		d_vpr_h("%s: pm already suspended\n", __func__);
> +		msm_vidc_change_core_sub_state(core, 0, CORE_SUBSTATE_PM_SUSPEND, __func__);
> +		rc = 0;
> +		goto unlock;
> +	} else if (allow != MSM_VIDC_ALLOW) {
> +		d_vpr_h("%s: pm suspend not allowed\n", __func__);
> +		rc = 0;
> +		goto unlock;
> +	}
> +
> +	rc = msm_vidc_suspend(core);
> +	if (rc == -EOPNOTSUPP)
> +		rc = 0;
> +	else if (rc)
> +		d_vpr_e("Failed to suspend: %d\n", rc);
> +	else
> +		msm_vidc_change_core_sub_state(core, 0, CORE_SUBSTATE_PM_SUSPEND, __func__);
> +
> +unlock:
> +	core_unlock(core, __func__);
> +	return rc;
> +}
> +
> +static int msm_vidc_pm_resume(struct device *dev)
Same comments as in _suspend

Konrad