Re: [PATCH 2/2] drm: Add new driver for MXSFB controller

From: Marek Vasut <marex@denx.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Fabio Estevam <fabio.estevam@nxp.com>,
	Shawn Guo <shawnguo@kernel.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm: Add new driver for MXSFB controller
Date: Sun, 25 Sep 2016 21:26:16 +0200	[thread overview]
Message-ID: <f3638b49-94d7-3c20-6b21-407515d2a74a@denx.de> (raw)
In-Reply-To: <20160828164449.GZ10980@phenom.ffwll.local>

On 08/28/2016 06:44 PM, Daniel Vetter wrote:
> On Fri, Aug 26, 2016 at 04:27:42PM +0200, Marek Vasut wrote:
>> Add new driver for the MXSFB controller found in i.MX23/28/6SX .
>> The MXSFB controller is a simple framebuffer controller with one
>> parallel LCD output. Unlike the MXSFB fbdev driver that is used
>> on these systems now, this driver uses the DRM/KMS framework.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
>> Cc: Shawn Guo <shawnguo@kernel.org>

Hi, sorry for the late reply.

[...]
>> +	switch (format->fourcc) {
>> +	case DRM_FORMAT_RGB565:
>> +		dev_dbg(drm->dev, "Setting up RGB565 mode\n");
>> +		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_16BIT);
>> +		ctrl |= CTRL_SET_WORD_LENGTH(0);
>> +		writel(CTRL1_SET_BYTE_PACKAGING(0xf), mxsfb->base + LCDC_CTRL1);
>> +		break;
>> +	case DRM_FORMAT_XRGB8888:
>> +		dev_dbg(drm->dev, "Setting up XRGB8888 mode\n");
>> +		ctrl |= CTRL_SET_BUS_WIDTH(STMLCDIF_24BIT);
>> +		ctrl |= CTRL_SET_WORD_LENGTH(3);
>> +		/* Do not use packed pixels = one pixel per word instead */
>> +		writel(CTRL1_SET_BYTE_PACKAGING(0x7), mxsfb->base + LCDC_CTRL1);
>> +		break;
>> +	default:
>> +		dev_err(drm->dev, "Unhandled color format %s\n",
>> +			format->name);
>> +		return -EINVAL;
> 
> You need to check such failures in the atomic_check code, doing it only in
> atomic_commit (or anything called from that) is way too late.
> 
> If you do check this already (simply restrict the list of support fourcc
> codes to only the 2 above), then you can do a WARN_ON + return; and change
> the return value from int to void here.

I now switched to drm_simple_kms_.* , which does that in the check, so
fixed.

>> +	}
>> +
>> +	writel(ctrl, mxsfb->base + LCDC_CTRL);
>> +	return 0;
>> +}

[...]

>> +static void mxsfb_crtc_mode_set_nofb(struct drm_crtc *crtc)
>> +{
>> +	struct mxsfb_drm_private *mxsfb = crtc_to_mxsfb_priv(crtc);
>> +	struct drm_display_mode *m = &crtc->state->adjusted_mode;
>> +	const u32 bus_flags = mxsfb->output.connector.display_info.bus_flags;
>> +	u32 vdctrl0, vsync_pulse_len, hsync_pulse_len;
>> +	bool reenable = false;
>> +	int err;
>> +
>> +	/*
>> +	 * It seems, you can't re-program the controller if it is still
>> +	 * running. This may lead to shifted pictures (FIFO issue?), so
>> +	 * first stop the controller and drain its FIFOs.
>> +	 */
>> +	if (mxsfb->enabled) {
>> +		reenable = true;
>> +		mxsfb_disable_controller(mxsfb);
>> +	}
> 
> The atomic core should keep perfect track of the state of your controller
> and never asky ou to re-enable it when it's already enabled. Please remove
> this code (and the ->enabled tracking, it should be redundant).
> 
> If this doesn't work then we have a bug in the atomic core.

What this code does is it temporarily disables the controller if it was
enabled when this function is invoked and re-enables it before exiting
this function. This is needed for this particular controller to
reconfigure it without odd misbehavior of the hardware itself.

Is there a better way ?

>> +
>> +	mxsfb_enable_axi_clk(mxsfb);
>> +
>> +	/* Clear the FIFOs */
>> +	writel(CTRL1_FIFO_CLEAR, mxsfb->base + LCDC_CTRL1 + REG_SET);

[...]

>> +static const struct drm_crtc_helper_funcs mxsfb_crtc_helper_funcs = {
>> +	.mode_set	= drm_helper_crtc_mode_set,
>> +	.mode_set_base	= drm_helper_crtc_mode_set_base,
>> +	.mode_set_nofb	= mxsfb_crtc_mode_set_nofb,
>> +	.enable		= mxsfb_crtc_enable,
>> +	.disable	= mxsfb_crtc_disable,
>> +	.prepare	= mxsfb_crtc_disable,
>> +	.commit		= mxsfb_crtc_enable,
>> +	.atomic_check	= mxsfb_crtc_atomic_check,
>> +	.atomic_begin	= mxsfb_crtc_atomic_begin,
>> +};
> 
> I think this driver is a perfect example for using the recently-merged
> drm_simple_kms_helper framework - it will allow you to remove a lot of the
> boiler-plate you have here. Please make sure you're using the latest
> drm-misc code in linux-next, since that contains an important fix to these
> helpers.
> 
> There's also some kernel-doc for these new helpers, which should help in
> converting your driver:
> 
> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#simple-kms-helper-reference

Done, thanks.

[...]

>> +static void mxsfb_fb_output_poll_changed(struct drm_device *drm)
>> +{
>> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
>> +
>> +	if (mxsfb->fbdev) {
>> +		drm_fbdev_cma_hotplug_event(mxsfb->fbdev);
>> +	} else {
>> +		mxsfb->fbdev = drm_fbdev_cma_init(drm, 32,
>> +					drm->mode_config.num_crtc,
>> +					drm->mode_config.num_connector);
>> +		if (IS_ERR(mxsfb->fbdev))
>> +			mxsfb->fbdev = NULL;
>> +	}
>> +}
> 
> There's patches from Thierry Redding to make delayed fbdev init supported
> in the fbdev helpers themselves (instead of reinventing it in every driver
> like you do here). Please help get those patches landed&reviewed, I'll
> ping Thierry to give you the relevant pointers.

It seems I won't even need this, since my output is not polled and
doesn't change. I moved the drm_fbdev_cma_init() into the mxsfb_load()
function.

>> +
>> +static int mxsfb_atomic_commit(struct drm_device *dev,
>> +			       struct drm_atomic_state *state, bool nonblock)
>> +{
>> +	return drm_atomic_helper_commit(dev, state, false);
> 
> Atomic helpers support async commit nowadays, no need any more for this
> hack.

I had to add the same fence check as imx drm driver, so this function
grew in V2.

>> +}
>> +
>> +static const struct drm_mode_config_funcs mxsfb_mode_config_funcs = {
>> +	.fb_create		= drm_fb_cma_create,
>> +	.output_poll_changed	= mxsfb_fb_output_poll_changed,
>> +	.atomic_check		= drm_atomic_helper_check,
>> +	.atomic_commit		= mxsfb_atomic_commit,
>> +};

[...]

>> +static int mxsfb_probe(struct platform_device *pdev)
>> +{
>> +	struct drm_device *drm;
>> +	const struct of_device_id *of_id =
>> +			of_match_device(mxsfb_dt_ids, &pdev->dev);
>> +	int ret;
>> +
>> +	if (!pdev->dev.of_node)
>> +		return -ENODEV;
>> +
>> +	if (of_id)
>> +		pdev->id_entry = of_id->data;
>> +
>> +	drm = drm_dev_alloc(&mxsfb_driver, &pdev->dev);
>> +	if (!drm)
>> +		return -ENOMEM;
>> +
>> +	ret = mxsfb_load(drm, 0);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	ret = drm_dev_register(drm, 0);
>> +	if (ret)
>> +		goto err_unload;
>> +
>> +	ret = drm_connector_register_all(drm);
> 
> No need anymore to call connector_register/unregister_all yourself. Please
> remove here and in the unload code.

Done, dropped.

>> +	if (ret) {
>> +		DRM_ERROR("Failed to bind all components\n");
>> +		goto err_unregister;
>> +	}
>> +
>> +	return 0;

[...]

Thanks!

-- 
Best regards,
Marek Vasut
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel