Re: [PATCH 9/9] drm/verisilicon: Add starfive hdmi driver

[Maxime Ripard]

Hi,

On Fri, Jun 02, 2023 at 03:40:43PM +0800, Keith Zhao wrote:
> Add HDMI dirver for StarFive SoC JH7110.
> 
> Signed-off-by: Keith Zhao <keith.zhao@xxxxxxxxxxxxxxxx>

I have a few high level comments:

> +static int starfive_hdmi_setup(struct starfive_hdmi *hdmi,
> +			       struct drm_display_mode *mode)
> +{
> +	hdmi_modb(hdmi, STARFIVE_BIAS_CONTROL, STARFIVE_BIAS_ENABLE, STARFIVE_BIAS_ENABLE);
> +	hdmi_writeb(hdmi, STARFIVE_RX_CONTROL, STARFIVE_RX_ENABLE);
> +	hdmi->hdmi_data.vic = drm_match_cea_mode(mode);
> +
> +	hdmi->tmds_rate = mode->clock * 1000;
> +	starfive_hdmi_phy_clk_set_rate(hdmi);
> +
> +	while (!(hdmi_readb(hdmi, STARFIVE_PRE_PLL_LOCK_STATUS) & 0x1))
> +		continue;
> +	while (!(hdmi_readb(hdmi, STARFIVE_POST_PLL_LOCK_STATUS) & 0x1))
> +		continue;
> +
> +	/*turn on LDO*/
> +	hdmi_writeb(hdmi, STARFIVE_LDO_CONTROL, STARFIVE_LDO_ENABLE);
> +	/*turn on serializer*/
> +	hdmi_writeb(hdmi, STARFIVE_SERIALIER_CONTROL, STARFIVE_SERIALIER_ENABLE);
> +
> +	starfive_hdmi_tx_phy_power_down(hdmi);
> +	starfive_hdmi_config_video_timing(hdmi, mode);
> +	starfive_hdmi_tx_phy_power_on(hdmi);
> +
> +	starfive_hdmi_tmds_driver_on(hdmi);
> +	starfive_hdmi_sync_tmds(hdmi);
> +
> +	return 0;
> +}

The PHY PLL supports rate until 594MHz, but I don't see any scrambler
setup here?

> +static void starfive_hdmi_encoder_mode_set(struct drm_encoder *encoder,
> +					   struct drm_display_mode *mode,
> +					   struct drm_display_mode *adj_mode)
> +{
> +	struct starfive_hdmi *hdmi = encoder_to_hdmi(encoder);
> +
> +	starfive_hdmi_setup(hdmi, adj_mode);

You should put that call into the enable callback, there's no need to
power it up at that point.

> +	memcpy(&hdmi->previous_mode, adj_mode, sizeof(hdmi->previous_mode));

You don't seem to be using that anywhere, and it's not the previous but
the current mode.

> +}
> +
> +static void starfive_hdmi_encoder_enable(struct drm_encoder *encoder)
> +{
> +	struct starfive_hdmi *hdmi = encoder_to_hdmi(encoder);
> +
> +	pm_runtime_get_sync(hdmi->dev);
> +}
> +
> +static void starfive_hdmi_encoder_disable(struct drm_encoder *encoder)
> +{
> +	struct starfive_hdmi *hdmi = encoder_to_hdmi(encoder);
> +
> +	pm_runtime_put(hdmi->dev);
> +}
> +
> +static bool starfive_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
> +					     const struct drm_display_mode *mode,
> +					     struct drm_display_mode *adj_mode)
> +{
> +	return true;
> +}

You can drop that one

> +static int
> +starfive_hdmi_encoder_atomic_check(struct drm_encoder *encoder,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
> +{
> +	return 0;
> +}

Ditto

> +static int starfive_hdmi_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct starfive_hdmi *hdmi = connector_to_hdmi(connector);
> +	struct edid *edid;
> +	int ret = 0;
> +
> +	if (!hdmi->ddc)
> +		return 0;
> +
> +	edid = drm_get_edid(connector, hdmi->ddc);
> +	if (edid) {
> +		hdmi->hdmi_data.sink_is_hdmi = drm_detect_hdmi_monitor(edid);
> +		hdmi->hdmi_data.sink_has_audio = drm_detect_monitor_audio(edid);
> +		drm_connector_update_edid_property(connector, edid);
> +		ret = drm_add_edid_modes(connector, edid);
> +		kfree(edid);
> +	}
> +
> +	return ret;
> +}

get_modes can be called while the connector is inactive, you need to
call pm_runtime_get_sync / pm_runtime_put here

> +static enum drm_mode_status
> +starfive_hdmi_connector_mode_valid(struct drm_connector *connector,
> +				   struct drm_display_mode *mode)
> +{
> +	const struct pre_pll_config *cfg = pre_pll_cfg_table;
> +	int pclk = mode->clock * 1000;
> +	bool valid = false;
> +	int i;
> +
> +	for (i = 0; cfg[i].pixclock != (~0UL); i++) {
> +		if (pclk == cfg[i].pixclock) {
> +			if (pclk > 297000000)
> +				continue;
> +
> +			valid = true;
> +			break;
> +		}
> +	}
> +
> +	return (valid) ? MODE_OK : MODE_BAD;
> +}

So I guess that's why you don't bother with the scrambler, you filter
all the modes > 297MHz?

If so, you also need to make sure it happens in atomic_check. mode_valid
will only filter the modes exposed to userspace, but the userspace is
free to send any mode it wants and that's checked by atomic_check.

> +
> +static int
> +starfive_hdmi_probe_single_connector_modes(struct drm_connector *connector,
> +					   u32 maxX, u32 maxY)
> +{
> +	struct starfive_hdmi *hdmi = connector_to_hdmi(connector);
> +	int ret;
> +
> +	pm_runtime_get_sync(hdmi->dev);
> +
> +	ret = drm_helper_probe_single_connector_modes(connector, 3840, 2160);
> +
> +	pm_runtime_put(hdmi->dev);
> +
> +	return ret;
> +}

You already have a pm_runtime_get_sync call in get_modes, why is that
necessary?

> +
> +static void starfive_hdmi_connector_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +}

Use drmm_connector_init.

> +static irqreturn_t starfive_hdmi_irq(int irq, void *dev_id)
> +{
> +	struct starfive_hdmi *hdmi = dev_id;
> +
> +	drm_helper_hpd_irq_event(hdmi->connector.dev);

drm_connector_helper_hpd_irq_event()

> +static int starfive_hdmi_get_clk_rst(struct device *dev, struct starfive_hdmi *hdmi)
> +{
> +	hdmi->sys_clk = devm_clk_get(dev, "sysclk");
> +	if (IS_ERR(hdmi->sys_clk)) {
> +		DRM_DEV_ERROR(dev, "Unable to get HDMI sysclk clk\n");
> +		return PTR_ERR(hdmi->sys_clk);
> +	}
> +	hdmi->mclk = devm_clk_get(dev, "mclk");
> +	if (IS_ERR(hdmi->mclk)) {
> +		DRM_DEV_ERROR(dev, "Unable to get HDMI mclk clk\n");
> +		return PTR_ERR(hdmi->mclk);
> +	}
> +	hdmi->bclk = devm_clk_get(dev, "bclk");
> +	if (IS_ERR(hdmi->bclk)) {
> +		DRM_DEV_ERROR(dev, "Unable to get HDMI bclk clk\n");
> +		return PTR_ERR(hdmi->bclk);
> +	}
> +	hdmi->tx_rst = reset_control_get_shared(dev, "hdmi_tx");
> +	if (IS_ERR(hdmi->tx_rst)) {
> +		DRM_DEV_ERROR(dev, "Unable to get HDMI tx rst\n");
> +		return PTR_ERR(hdmi->tx_rst);
> +	}

That one isn't device-managed, you'll need to put back the reference in
unbind.

> +	return 0;
> +}
> +
> +static int starfive_hdmi_bind(struct device *dev, struct device *master,
> +			      void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct starfive_hdmi *hdmi;
> +	struct resource *iores;
> +	int irq;
> +	int ret;
> +
> +	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> +	if (!hdmi)
> +		return -ENOMEM;

Using device-managed actions to allocate memory that will eventually
hold the connectors and encoders is unsafe.

Please use drmm_kzalloc here, and test that it all works fine by
enabling KASAN and removing the module.

> +
> +	hdmi->dev = dev;
> +	hdmi->drm_dev = drm;
> +
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	hdmi->regs = devm_ioremap_resource(dev, iores);
> +	if (IS_ERR(hdmi->regs))
> +		return PTR_ERR(hdmi->regs);

The main issue I was mentioning above is that whenever the device is
unbound from its driver, all the device-managed actions are executed.

However, the KMS device will still be there until the last (userspace)
user closes its FD, so if anything happens between the time the module
is removed and the FD is closed, you get plenty of use-after-free errors.

For MMIO accesses, this is even more true since you need to use a
device-managed action for the registers mapping (this is true for any
resource tied to the device itself, so clocks, reset, etc. fit that
description too).

To protect against it, you need to protect any device access by a call
to drm_dev_enter/drm_dev_exit.

> +
> +	ret = starfive_hdmi_get_clk_rst(dev, hdmi);
> +	ret = starfive_hdmi_enable_clk_deassert_rst(dev, hdmi);

Why does the device need to be powered here?

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = irq;
> +		goto err_disable_clk;
> +	}
> +
> +	hdmi->ddc = starfive_hdmi_i2c_adapter(hdmi);
> +	if (IS_ERR(hdmi->ddc)) {
> +		ret = PTR_ERR(hdmi->ddc);
> +		hdmi->ddc = NULL;
> +		goto err_disable_clk;
> +	}
> +
> +	hdmi->tmds_rate = clk_get_rate(hdmi->sys_clk);

It's not clear to me what tmds_rate is here, wouldn't that change from
one mode to the next?

> +	starfive_hdmi_i2c_init(hdmi);
> +
> +	ret = starfive_hdmi_register(drm, hdmi);
> +	if (ret)
> +		goto err_put_adapter;
> +
> +	dev_set_drvdata(dev, hdmi);
> +
> +	/* Unmute hotplug interrupt */
> +	hdmi_modb(hdmi, HDMI_STATUS, m_MASK_INT_HOTPLUG, v_MASK_INT_HOTPLUG(1));
> +
> +	ret = devm_request_threaded_irq(dev, irq, starfive_hdmi_hardirq,
> +					starfive_hdmi_irq, IRQF_SHARED,
> +					dev_name(dev), hdmi);
> +	if (ret < 0)
> +		goto err_cleanup_hdmi;
> +
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 500);

Autosuspend? Shouldn't we enable the device as long as there is an
active video output (and you have that covered already)?

> +	pm_runtime_enable(&pdev->dev);
> +
> +	starfive_hdmi_disable_clk_assert_rst(dev, hdmi);

It would be clearer if you would move
starfive_hdmi_enable_clk_deassert_rst()/disable_clk_assert_rst() into
runtime_resume/runtime_suspend, and then in you bind just call
pm_runtime_enable(), pm_runtime_get_sync(), do the registration, and
pm_runtime_put.

> +#define UPDATE(x, h, l)\
> +({\
> +	typeof(x) x_ = (x);\
> +	typeof(h) h_ = (h);\
> +	typeof(l) l_ = (l);\
> +	(((x_) << (l_)) & GENMASK((h_), (l_)));\
> +})

That's FIELD_PREP, right?
Maxime

Attachment: signature.asc
Description: PGP signature