Re: [v3 3/6] drm/vs: Register DRM device

[Keith Zhao <keith.zhao@starfivetech.com>]

hi Philipp:

On 2023/12/4 21:30, Philipp Zabel wrote:
> Hi Keith,
> 
> On Mo, 2023-12-04 at 20:33 +0800, Keith Zhao wrote:
>> Implement drm device registration interface
>> 
>> Signed-off-by: Keith Zhao <keith.zhao@starfivetech.com>
>> ---
> [...]
>> diff --git a/drivers/gpu/drm/verisilicon/Kconfig b/drivers/gpu/drm/verisilicon/Kconfig
>> new file mode 100644
>> index 000000000000..e10fa97635aa
>> --- /dev/null
>> +++ b/drivers/gpu/drm/verisilicon/Kconfig
>> @@ -0,0 +1,13 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +config DRM_VERISILICON
>> +	tristate "DRM Support for VeriSilicon"
>> +	depends on DRM
>> +	select DRM_KMS_HELPER
>> +	select DRM_GEM_DMA_HELPER
>> +	select CMA
>> +	select DMA_CMA
>> +	help
>> +	  Choose this option if you have a VeriSilicon soc chipset.
> 
> This seems a bit generic. Doesn't the VeriSilicon display controller IP
> used on JH7110 have a product name?
yes , there is a product name "dc8200", I will match it.
> 
> [...]
>> diff --git a/drivers/gpu/drm/verisilicon/vs_drv.c b/drivers/gpu/drm/verisilicon/vs_drv.c
>> new file mode 100644
>> index 000000000000..4fb1f29ef84b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/verisilicon/vs_drv.c
>> @@ -0,0 +1,316 @@
>> +// SPDX-License-Identifier: GPL-2.0
> [...]
>> +static void vs_drm_device_release_clocks(void *res)
>> +{
>> +	struct vs_drm_device *priv = res;
>> +	unsigned int i;
>> +
>> +	reset_control_bulk_assert(priv->nrsts, priv->rst_vout);
>> +
>> +	for (i = 0; i < priv->clk_count; ++i) {
>> +		if (priv->clks[i]) {
>> +			clk_disable_unprepare(priv->clks[i]);
>> +			clk_put(priv->clks[i]);
>> +		}
>> +	}
> 
> Why not use the bulk API for clk as well?
ok , will do it next version
> 
> [...]
>> +static int vs_drm_device_init_clocks(struct vs_drm_device *priv)
>> +{
>> +	struct drm_device *dev = &priv->base;
>> +	struct platform_device *pdev = to_platform_device(dev->dev);
>> +	struct device_node *of_node = pdev->dev.of_node;
>> +	struct clk *clock;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	if (dev_get_platdata(&pdev->dev) || !of_node)
>> +		return 0;
>> +
>> +	priv->nrsts = ARRAY_SIZE(priv->rst_vout);
>> +	for (int i = 0; i < priv->nrsts; ++i)
>> +		priv->rst_vout[i].id = vout_resets[i];
>> +	ret = devm_reset_control_bulk_get_shared(dev->dev, priv->nrsts,
>> +						 priv->rst_vout);
> 
> I would request resets and clocks in _probe().

> 
> If component_bind_all() returns -EPROBE_DEFER because of a still
> missing DSI panel backlight or similar, this doesn't have to be done
> multiple times.
I got what you mean. component_bind_all should be done multiple times
 to prevent the dsi panel driver from lagging load.

in my drm subsystem , there are 2 pipeline 

          +------------------------------+
          |                              |
          |                              |
  +----+  |   +-------------------+      |   +-------+   +------+   +------+
  |    +----->+  dc controller 0  +--->----->+HDMICtl| ->+ PHY  +-->+PANEL0+
  |AXI |  |   +-------------------+      |   +-------+   +------+   +------+
  |    |  |                              |
  |    |  |                              |
  |    |  |                              |
  |    |  |                              |
  |APB |  |   +-------------------+         +---------+    +------+  +-------+
  |    +----->+  dc controller 1  +--->---->+ dsiTx   +--->+DPHY  +->+ PANEL1+
  |    |  |   +-------------------+         +---------+    +------+  +-------+
  +----+  |                              |
          +------------------------------+


component_bind_all will bind the hdmi encoder and dsi encoder .
binding the hdmi encoder will always return ok .

binging the dsi encoder has a question :
I used the panel-raspberrypi-touchscreen.c as panel driver , 
this driver is a i2c device and it use a i2c command to read reg ID
if read success , it will do drm_panel_add. 

if I disconnect the panel ,it will not do drm_panel_add.
dsiTx will fail to find panel , The consequence is that the inputbridge cannot be created , 
also outputbridge cannot be created.
for encoder bind , it will fail to find the input bridge of dsi.
Under this premise, although returning -EPROBE_DEFER allows bind to be executed multiple times, 
the final result is that the entire bind fails.

returning -EPROBE_DEFER can solve panel driver from lagging load , 
but for no panel case , it will destory all pipeline (include hdmi and dsi).

I did two things:
late_initcall_sync(vs_drm_init); to make sure the panel drive has been probed;
dsi encoder bind always return ok to make sure hdmi pipeline ok at lease.
component_bind_all do once . 




> 
>> +	if (ret) {
>> +		drm_err(dev, "Failed to get reset controls\n");
>> +		return ret;
>> +	}
>> +
>> +	priv->clk_count = of_clk_get_parent_count(of_node);
>> +	if (!priv->clk_count)
>> +		return 0;
>> +
>> +	priv->clks = drmm_kzalloc(dev, priv->clk_count * sizeof(priv->clks[0]),
>> +				  GFP_KERNEL);
>> +	if (!priv->clks)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < priv->clk_count; ++i) {
>> +		clock = of_clk_get(of_node, i);
>> +		if (IS_ERR(clock)) {
>> +			ret = PTR_ERR(clock);
>> +			if (ret == -EPROBE_DEFER)
>> +				goto err;
>> +			drm_err(dev, "clock %u not found: %d\n", i, ret);
>> +			continue;
>> +		}
>> +		ret = clk_prepare_enable(clock);
>> +		if (ret) {
>> +			drm_err(dev, "failed to enable clock %u: %d\n",
>> +				i, ret);
>> +			clk_put(clock);
>> +			continue;
>> +		}
>> +		priv->clks[i] = clock;
>> +	}
>> +
>> +	ret = reset_control_bulk_deassert(priv->nrsts, priv->rst_vout);
>> +	if (ret)
>> +		return ret;
> 
> This should goto err, otherwise clocks are left enabled.
> 
>> +
>> +	return devm_add_action_or_reset(&pdev->dev,
>> +					vs_drm_device_release_clocks,
>> +					priv);
>> +
>> +err:
>> +	while (i) {
>> +		--i;
>> +		if (priv->clks[i]) {
>> +			clk_disable_unprepare(priv->clks[i]);
>> +			clk_put(priv->clks[i]);
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int vs_drm_bind(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct vs_drm_device *priv;
>> +	int ret;
>> +	struct drm_device *drm_dev;
>> +
>> +	/* Remove existing drivers that may own the framebuffer memory. */
>> +	ret = drm_aperture_remove_framebuffers(&vs_drm_driver);
>> +	if (ret)
>> +		return ret;
>> +
>> +	priv = devm_drm_dev_alloc(dev, &vs_drm_driver, struct vs_drm_device, base);
>> +	if (IS_ERR(priv))
>> +		return PTR_ERR(priv);
>> +
>> +	priv->pitch_alignment = 64;
> 
> Why is this a variable instead of a constant?
the dc controllers of VeriSilicon have different series, like dc8200 , dc9200...
Their pitch values may vary from series to series, like 64 , 128

> 
>> +	ret = dma_set_coherent_mask(priv->base.dev, DMA_BIT_MASK(40));
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_dev = &priv->base;
>> +	platform_set_drvdata(pdev, drm_dev);
>> +
>> +	ret = vs_drm_device_init_clocks(priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	vs_mode_config_init(drm_dev);
>> +
>> +	/* Now try and bind all our sub-components */
>> +	ret = component_bind_all(dev, drm_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = drm_vblank_init(drm_dev, drm_dev->mode_config.num_crtc);
>> +	if (ret)
>> +		return ret;
> 
> Missing component_unbind_all(), see below.
> 
>> +
>> +	drm_mode_config_reset(drm_dev);
>> +
>> +	drm_kms_helper_poll_init(drm_dev);
>> +
>> +	ret = drm_dev_register(drm_dev, 0);
>> +	if (ret)
>> +		return ret;
> 
> Missing drm_kms_helper_poll_fini(), see below.
> 
>> +
>> +	drm_fbdev_generic_setup(drm_dev, 32);
>> +
>> +	return 0;
> 
> Here I'd expect an error path calling drm_kms_helper_poll_fini() and
> component_unbind_all() as appropriate.
> 
ok

> 
> regards
> Philipp