xilinx_drm vblank from VDMA callback

[Jean-Francois Dagenais]

> On Jul 5, 2017, at 18:56, Hyun Kwon <hyun.kwon at xilinx.com> wrote:
> 
> For VDMA, I think you can use the DMA engine complete callback to generate the vblank.
> 
> But, please note that it's not a global solution, and it doesn't work for some other pipelines. For example, the ZU+ DPDMA operation is synchronized with vblank, not with the done event. Thus if the page flip request comes in between done event and vblank, it'll end up flipping pages while the application would expect the flip to take place after vblank.

Thanks for that warning. I will make sure I get the timing right for my pipeline. I will probably not submit this work upstream... yet.

> 
> For the xilinx drm clean up, what you described in the previous email, to use drm_of_component_probe(), makes sense. I'm doing the clean in that direction too.

In the end I did not use drm_of_component_probe() as it make assumptions that the CRTC is a component and treats it specially, failing that, the function aborts. Since I didn't want to refactor xilinx_drm_crtc to a component at this stage, especially knowing the refactoring that is upcoming, I went for a similar but manual way.

Here it is, just for fun... 

/* component master bind callback */
static int xilinx_drm_bind(struct device *dev)
{
	struct drm_device *drm;
	struct xilinx_drm_private *private;
	struct drm_encoder *encoder;
	struct drm_connector *connector;
	struct device_node *encoder_node;
	unsigned int bpp, align, i = 0;
	int ret;

	private = devm_kzalloc(dev, sizeof(*private), GFP_KERNEL);
	if (!private)
		return -ENOMEM;

	dev_set_drvdata(dev, private);

	drm = drm_dev_alloc(&xilinx_drm_driver, dev);
	if (IS_ERR(drm))
		return PTR_ERR(drm);

	drm->dev_private = private;
	private->drm = drm;
	platform_set_drvdata(to_platform_device(dev), private);

	drm_mode_config_init(drm);

	/* create a xilinx crtc */
	private->crtc = xilinx_drm_crtc_create(drm);
	if (IS_ERR(private->crtc)) {
		DRM_DEBUG_DRIVER("failed to create xilinx crtc\n");
		ret = PTR_ERR(private->crtc);
		goto err_free;
	}

	xilinx_drm_mode_config_init(drm);

	while ((encoder_node = of_parse_phandle(drm->dev->of_node,
						"xlnx,encoder-slave", i))) {
		encoder = xilinx_drm_encoder_create(drm, encoder_node);
		of_node_put(encoder_node);
		if (IS_ERR(encoder)) {
			DRM_DEBUG_DRIVER("failed to create xilinx encoder\n");
			ret = PTR_ERR(encoder);
			goto err_free;
		}

		connector = xilinx_drm_connector_create(drm, encoder, i);
		if (IS_ERR(connector)) {
			DRM_DEBUG_DRIVER("failed to create xilinx connector\n");
			ret = PTR_ERR(connector);
			goto err_free;
		}

		i++;
	}

	ret = drm_dev_register(drm, 0);
	if (ret) {
		DRM_ERROR("failed to register drm_dev: %d\n", ret);
		goto err_free;
	}

	ret = component_bind_all(dev, drm);
	if (ret) {
		DRM_ERROR("Failed to bind all components\n");
		goto err_unregister;
	}

	ret = drm_vblank_init(drm, 1);
	if (ret) {
		dev_err(dev, "failed to initialize vblank\n");
		goto err_unbind_all;
	}

	/* enable irq to enable vblank feature */
	drm->irq_enabled = true;

	/* initialize xilinx framebuffer */
	bpp = xilinx_drm_format_bpp(xilinx_drm_crtc_get_format(private->crtc));
	align = xilinx_drm_crtc_get_align(private->crtc);
	private->fb = xilinx_drm_fb_init(drm, bpp, 1, 1, align,
					 xilinx_drm_fbdev_vres);
	if (IS_ERR(private->fb)) {
		DRM_ERROR("failed to initialize drm cma fb\n");
		ret = PTR_ERR(private->fb);
		goto err_vblank_cleanup;
	}

	drm_helper_disable_unused_functions(drm);
	drm_mode_config_reset(drm);
	drm_kms_helper_poll_init(drm);

	return 0;

err_vblank_cleanup:
	drm_vblank_cleanup(drm);
err_unbind_all:
	component_unbind_all(dev, drm);
err_unregister:
	drm_dev_unregister(drm);
err_free:
	drm_mode_config_cleanup(drm);
	dev_set_drvdata(dev, NULL);
	drm_dev_unref(drm);

	return ret;
}

static void xilinx_drm_unbind(struct device *dev)
{
	struct xilinx_drm_private *private = dev_get_drvdata(dev);
	struct drm_device *drm = private->drm;

	drm_kms_helper_poll_fini(drm);
	xilinx_drm_fb_fini(private->fb);
	component_unbind_all(dev, drm);
	drm_vblank_cleanup(drm);
	drm_mode_config_cleanup(drm);
	drm_dev_unregister(drm);
	drm_dev_unref(drm);
	drm->dev_private = NULL;
	dev_set_drvdata(dev, NULL);
}

static const struct component_master_ops xilinx_drm_component_master_ops = {
	.bind	= xilinx_drm_bind,
	.unbind	= xilinx_drm_unbind,
};

static int compare_dev(struct device *dev, void *data)
{
	return dev->of_node == data;
}

/* init xilinx drm platform */
static int xilinx_drm_platform_probe(struct platform_device *pdev)
{
	struct device_node *remote_dev, *ep = NULL;
	struct component_match *match = NULL;

	while (1) {
		ep = of_graph_get_next_endpoint(pdev->dev.of_node, ep);
		if (!ep)
			break;

		of_node_put(ep);
		remote_dev = of_graph_get_remote_port_parent(ep);
		if (!remote_dev || !of_device_is_available(remote_dev)) {
			of_node_put(remote_dev);
			continue;
		}

		component_match_add(&pdev->dev, &match, compare_dev, remote_dev);
		of_node_put(remote_dev);
	}

	return component_master_add_with_match(&pdev->dev,
					       &xilinx_drm_component_master_ops,
					       match);
}

/* exit xilinx drm platform */
static int xilinx_drm_platform_remove(struct platform_device *pdev)
{
	component_master_del(&pdev->dev, &xilinx_drm_component_master_ops);

	return 0;
}


There is still a bad pointer dereference at unbind time which doesn't annoy me enough yet since I never unbind, I just shutdown the device.

> I can post my local branch to github and point it to you, even though I'm not sure how much it'd be helpful as it's more like scribble for now. Then I can also include you when I post the patch to Xilinx mailing list for internal review, which I expect to happen in July timeframe.

You can flag your commit with "WIP: ..." to warn anyone looking, and detail your commit with your concerns and challenges. Onlookers may just chime in to provide insights ;)

It's a good timing for this work since I am actively building our support for our display pipeline which is not covered by the current driver's state. If we collaborate, I will want to submit my changes to allow others wanting to integrate LVDS panels in xilinx_drm to do so. Right now, with xilinx_drm using the legacy helpers, my approach requires a small hack in an atomic helper function.

Cheers!