Re: [PATCH v11 4/7] drm/imx: Add support for i.MX94 DCIF

[Luca Ceresoli]

Hello Laurentiu,

> The i.MX94 Display Control Interface features:
>  * Up to maximum 3 layers of alpha blending:
>     - 1 background layer(Layer 0);
>     - 1 foreground layer(Layer 1);
>     - A programmable constant color behind the background layer;
>  * Each layer supports:
>     - programmable plane size;
>     - programmable background color;
>     - embedded alpha and global alpha;
>  * Data output with CRC checksum for 4 programmable regions;
>
> Reviewed-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> # bridge refcounting
> Signed-off-by: Sandor Yu <sandor.yu@xxxxxxx>
> Co-developed-by: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxxxx>
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxxxx>

Looks OK, I just have a small remark and a few minor nitpicks, see below.

> +++ b/drivers/gpu/drm/imx/dcif/Kconfig
> @@ -0,0 +1,16 @@
> +config DRM_IMX_DCIF
> +	tristate "DRM support for NXP i.MX94 DCIF"
> +	select DRM_KMS_HELPER
> +	select DRM_GEM_DMA_HELPER
> +	select DRM_DISPLAY_HELPER
> +	select DRM_PANEL_BRIDGE

This should be unneeded now. As o fnow it is basically a complicated way to
say 'select DRM_PANEL'. And you are (rightfully) not using the panel API,
so this line should probably just be removed. Please double check by
building multiple configs, including an allmodconfig.

> +++ b/drivers/gpu/drm/imx/dcif/dcif-crc.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2025 NXP

2026 I guess... v11, a long journey, hopefully it won't slip to 2027! :)

> +++ b/drivers/gpu/drm/imx/dcif/dcif-crtc.c
...
> +static int dcif_crtc_query_output_bus_format(struct drm_crtc *crtc,
> +					     struct drm_crtc_state *crtc_state)
> +{
> +	struct dcif_crtc_state *dcif_state = to_dcif_crtc_state(crtc_state);
> +	struct drm_bridge *bridge __free(drm_bridge_put) = NULL;

While this is OK, there is generally a preference to declare and assign in
the same statement when using cleanup actions, so...

> +	struct drm_bridge_state *bridge_state;
> +	u32 bus_format;
> +
> +	dcif_state->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	dcif_state->bus_flags = 0;
> +
> +	bridge = dcif_crtc_get_bridge(crtc, crtc_state);

...it should all happen here, as:

	struct drm_bridge *bridge __free(drm_bridge_put) =
		dcif_crtc_get_bridge(crtc, crtc_state);

> +++ b/drivers/gpu/drm/imx/dcif/dcif-kms.c
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_panel.h>

Unused include?

> +static int dcif_kms_init(struct dcif_dev *dcif)
> +{
> +	struct drm_device *drm = &dcif->drm;
> +	struct device_node *np = drm->dev->of_node;
> +	struct drm_connector *connector;
> +	struct drm_bridge *bridge;
> +	int ret;
> +
> +	ret = dcif_crtc_init(dcif);
> +	if (ret)
> +		return ret;
> +
> +	bridge = devm_drm_of_get_bridge(drm->dev, np, 0, 0);

This function is deprecated. You can use of_drm_get_bridge_by_endpoint()
instead, which returns a refcounted bridge pointer. And since the bridge
pointer is a local variable you can just use a cleanup action to put it
easily:

  struct drm_bridge *bridge __free(drm_bridge_put) = of_drm_get_bridge_by_endpoint(...);

Otherwise LGTM.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com