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 at denx.de>
>> Cc: Lucas Stach <l.stach at pengutronix.de>
>> Cc: Fabio Estevam <fabio.estevam at nxp.com>
>> Cc: Shawn Guo <shawnguo at 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
