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
