RE: [PATCH v9 RESEND 4/5] drm: Add RZ/G2L DU Support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH v9 RESEND 4/5] drm: Add RZ/G2L DU Support
> 
> Hi Biju,
> 
> Thank you for the patch.
> 
> This is a partial review, because the driver is big, and because some
> changes in v10 will (hopefully) simplify the code and make review
> easier.

I agree v10 will simplify the code as I have do clean-ups based on your
review commnet.

> 
> On Tue, May 02, 2023 at 11:09:11AM +0100, Biju Das wrote:
> > The LCD controller is composed of Frame Compression Processor (FCPVD),
> > Video Signal Processor (VSPD), and Display Unit (DU).
> >
> > It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> > along with 2 RPFs to support the blending of two picture layers and
> > raster operations (ROPs).
> >
> > The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> > alike SoCs.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > Ref:
> >
> > v8->v9:
> >  * Dropped reset_control_assert() from error patch for
> rzg2l_du_crtc_get() as
> >    suggested by Philipp Zabel.
> > v7->v8:
> >  * Dropped RCar du lib and created RZ/G2L DU DRM driver by creating
> rz_du folder.
> >  * Updated KConfig and Makefile.
> > v6->v7:
> >  * Split DU lib and  RZ/G2L du driver as separate patch series as
> >    DU support added to more platforms based on RZ/G2L alike SoCs.
> >  * Rebased to latest drm-tip.
> >  * Added patch #2 for binding support for RZ/V2L DU
> >  * Added patch #4 for driver support for RZ/V2L DU
> >  * Added patch #5 for SoC DTSI support for RZ/G2L DU
> >  * Added patch #6 for SoC DTSI support for RZ/V2L DU
> >  * Added patch #7 for Enabling DU on SMARC EVK based on RZ/{G2L,V2L}
> SoCs.
> >  * Added patch #8 for Enabling DU on SMARC EVK based on RZ/G2LC SoC.
> > ---
> >  drivers/gpu/drm/renesas/Kconfig               |   1 +
> >  drivers/gpu/drm/renesas/Makefile              |   1 +
> >  drivers/gpu/drm/renesas/rz-du/Kconfig         |  20 +
> >  drivers/gpu/drm/renesas/rz-du/Makefile        |   8 +
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c | 714 ++++++++++++++++
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h |  99 +++
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c  | 188 +++++
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h  |  89 ++
> >  .../gpu/drm/renesas/rz-du/rzg2l_du_encoder.c  | 112 +++
> >  .../gpu/drm/renesas/rz-du/rzg2l_du_encoder.h  |  28 +
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c  | 770
> ++++++++++++++++++
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.h  |  43 +
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_regs.h |  67 ++
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.c  | 430 ++++++++++
> >  drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.h  |  94 +++
> >  15 files changed, 2664 insertions(+)
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/Kconfig
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/Makefile
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.c
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_encoder.h
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.c
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_kms.h
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_regs.h
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.c
> >  create mode 100644 drivers/gpu/drm/renesas/rz-du/rzg2l_du_vsp.h
> >
> > diff --git a/drivers/gpu/drm/renesas/Kconfig
> b/drivers/gpu/drm/renesas/Kconfig
> > index 3777dad17f81..21862a8ef710 100644
> > --- a/drivers/gpu/drm/renesas/Kconfig
> > +++ b/drivers/gpu/drm/renesas/Kconfig
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >
> >  source "drivers/gpu/drm/renesas/rcar-du/Kconfig"
> > +source "drivers/gpu/drm/renesas/rz-du/Kconfig"
> >  source "drivers/gpu/drm/renesas/shmobile/Kconfig"
> > diff --git a/drivers/gpu/drm/renesas/Makefile
> b/drivers/gpu/drm/renesas/Makefile
> > index ec0e89e7a592..b8d8bc53967f 100644
> > --- a/drivers/gpu/drm/renesas/Makefile
> > +++ b/drivers/gpu/drm/renesas/Makefile
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >
> >  obj-y += rcar-du/
> > +obj-y += rz-du/
> >  obj-$(CONFIG_DRM_SHMOBILE) += shmobile/
> > diff --git a/drivers/gpu/drm/renesas/rz-du/Kconfig
> b/drivers/gpu/drm/renesas/rz-du/Kconfig
> > new file mode 100644
> > index 000000000000..90b1bf72e23b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/Kconfig
> > @@ -0,0 +1,20 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +config DRM_RZG2L_DU
> > +	tristate "DRM Support for RZ/G2L Display Unit"
> > +	depends on DRM && OF
> > +	depends on ARM64
> 
> Does the driver fail to compile on !ARM64 platforms ? If no, I'd drop
> this.

Agreed.

> 
> > +	depends on DRM_RCAR_VSP
> > +	depends on ARCH_RZG2L || COMPILE_TEST
> > +	select DRM_KMS_HELPER
> > +	select DRM_GEM_DMA_HELPER
> 
> Alphabetical order please.

Ok.

> 
> > +	select VIDEOMODE_HELPERS
> > +	help
> > +	  Choose this option if you have an RZ/G2L alike chipset.
> > +	  If M is selected the module will be called rzg2l-du-drm.
> > +
> > +config DRM_RCAR_VSP
> > +	bool "R-Car DU VSP Compositor Support" if ARM
> > +	default y if ARM64
> > +	depends on VIDEO_RENESAS_VSP1
> > +	help
> > +	  Enable support to expose the R-Car VSP Compositor as KMS planes.
> 
> This duplicates the config symbol in
> drivers/gpu/drm/renesas/rcar-du/Kconfig.
> 
> Unlike on R-Car, where some SoC generations can operate without the VSP,
> RZ/G2L requires the VSP. You can drop this configuration option and just
> make DRM_RZG2L_DU depend on VIDEO_RENESAS_VSP1.

OK, Will make DRM_RZG2L_DU depend on VIDEO_RENESAS_VSP1.

> 
> > diff --git a/drivers/gpu/drm/renesas/rz-du/Makefile
> b/drivers/gpu/drm/renesas/rz-du/Makefile
> > new file mode 100644
> > index 000000000000..2cdf3ccd0459
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/Makefile
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +rzg2l-du-drm-y := rzg2l_du_crtc.o \
> > +		  rzg2l_du_drv.o \
> > +		  rzg2l_du_encoder.o \
> > +		  rzg2l_du_kms.o \
> > +
> > +rzg2l-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rzg2l_du_vsp.o
> > +obj-$(CONFIG_DRM_RZG2L_DU)		+= rzg2l-du-drm.o
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > new file mode 100644
> > index 000000000000..d61d433d72e6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.c
> > @@ -0,0 +1,714 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * RZ/G2L Display Unit CRTCs
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + *
> > + * Based on rcar_du_crtc.c
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <drm/drm_gem_dma_helper.h>
> > +#include <drm/drm_vblank.h>
> > +
> > +#include "rzg2l_du_crtc.h"
> > +#include "rzg2l_du_drv.h"
> > +#include "rzg2l_du_encoder.h"
> > +#include "rzg2l_du_kms.h"
> > +#include "rzg2l_du_vsp.h"
> > +#include "rzg2l_du_regs.h"
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * Hardware Setup
> > + */
> > +
> > +static void rzg2l_du_crtc_set_display_timing(struct rzg2l_du_crtc
> *rcrtc)
> > +{
> > +	const struct drm_display_mode *mode = &rcrtc->crtc.state-
> >adjusted_mode;
> > +	struct rzg2l_du_device *rcdu = rcrtc->dev;
> > +	unsigned long mode_clock = mode->clock * 1000;
> > +	u32 ditr0, ditr1, ditr2, ditr3, ditr4, ditr5, pbcr0;
> > +	struct clk *parent_clk;
> > +
> > +	parent_clk = clk_get_parent(rcrtc->rzg2l_clocks.dclk);
> > +	clk_set_rate(parent_clk, mode_clock);
> 
> Shouldn't the clock framework configure the parent correctly if you set
> the dclk rate ?

Yes it will do.

> 
> > +
> > +	clk_prepare_enable(rcrtc->rzg2l_clocks.dclk);
> > +
> > +	ditr0 = (DU_DITR0_DEMD_HIGH
> > +		 | ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DU_DITR0_VSPOL :
> 0)
> > +		 | ((mode->flags & DRM_MODE_FLAG_PHSYNC) ? DU_DITR0_HSPOL :
> 0));
> 
> No need for the outer parentheses.
> 
> I usually align the | under the =, but that's up to you.

OK will align | under the =

> 
> > +
> > +	ditr1 = DU_DITR1_VSA(mode->vsync_end - mode->vsync_start)
> > +		| DU_DITR1_VACTIVE(mode->vdisplay);
> > +
> > +	ditr2 = DU_DITR2_VBP(mode->vtotal - mode->vsync_end)
> > +		| DU_DITR2_VFP(mode->vsync_start - mode->vdisplay);
> > +
> > +	ditr3 = DU_DITR3_HSA(mode->hsync_end - mode->hsync_start)
> > +		| DU_DITR3_HACTIVE(mode->hdisplay);
> > +
> > +	ditr4 = DU_DITR4_HBP(mode->htotal - mode->hsync_end)
> > +		| DU_DITR4_HFP(mode->hsync_start - mode->hdisplay);
> > +
> > +	ditr5 = DU_DITR5_VSFT(0) | DU_DITR5_HSFT(0);
> > +
> > +	pbcr0 = DU_PBCR0_PB_DEP(0x1f);
> > +
> > +	writel(ditr0, rcdu->mmio + DU_DITR0);
> 
> Please implement read/write wrappers that take an rcdu pointer and add
> the offset. It will simplify the callers.

I have implemented write as there is no user for read.

> 
> > +	writel(ditr1, rcdu->mmio + DU_DITR1);
> > +	writel(ditr2, rcdu->mmio + DU_DITR2);
> > +	writel(ditr3, rcdu->mmio + DU_DITR3);
> > +	writel(ditr4, rcdu->mmio + DU_DITR4);
> > +	writel(ditr5, rcdu->mmio + DU_DITR5);
> > +	writel(pbcr0, rcdu->mmio + DU_PBCR0);
> > +
> > +	/* Enable auto resume when underrun */
> > +	writel(DU_MCR1_PB_AUTOCLR, rcdu->mmio + DU_MCR1);
> > +}
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * Page Flip
> > + */
> > +
> > +void rzg2l_du_crtc_finish_page_flip(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	struct drm_pending_vblank_event *event;
> > +	struct drm_device *dev = rcrtc->crtc.dev;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	event = rcrtc->event;
> > +	rcrtc->event = NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	if (!event)
> > +		return;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	drm_crtc_send_vblank_event(&rcrtc->crtc, event);
> > +	wake_up(&rcrtc->flip_wait);
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	drm_crtc_vblank_put(&rcrtc->crtc);
> > +}
> > +
> > +static bool rzg2l_du_crtc_page_flip_pending(struct rzg2l_du_crtc
> *rcrtc)
> > +{
> > +	struct drm_device *dev = rcrtc->crtc.dev;
> > +	unsigned long flags;
> > +	bool pending;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = rcrtc->event;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	return pending;
> > +}
> > +
> > +static void rzg2l_du_crtc_wait_page_flip(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	struct rzg2l_du_device *rcdu = rcrtc->dev;
> > +
> > +	if (wait_event_timeout(rcrtc->flip_wait,
> > +			       !rzg2l_du_crtc_page_flip_pending(rcrtc),
> > +			       msecs_to_jiffies(50)))
> > +		return;
> > +
> > +	dev_warn(rcdu->dev, "page flip timeout\n");
> > +
> > +	rzg2l_du_crtc_finish_page_flip(rcrtc);
> > +}
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * Start/Stop and Suspend/Resume
> > + */
> > +
> > +static void rzg2l_du_crtc_setup(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	/* Configure display timings and output routing */
> > +	rzg2l_du_crtc_set_display_timing(rcrtc);
> > +
> > +	/* Enable the VSP compositor. */
> > +	rzg2l_du_vsp_enable(rcrtc);
> > +
> > +	/* Turn vertical blanking interrupt reporting on. */
> > +	drm_crtc_vblank_on(&rcrtc->crtc);
> > +}
> > +
> > +static int rzg2l_du_crtc_get(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * Guard against double-get, as the function is called from both
> the
> > +	 * .atomic_enable() and .atomic_begin() handlers.
> > +	 */
> > +	if (rcrtc->initialized)
> > +		return 0;
> > +
> > +	ret = clk_prepare_enable(rcrtc->rzg2l_clocks.aclk);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(rcrtc->rzg2l_clocks.pclk);
> > +	if (ret < 0)
> > +		goto error_bus_clock;
> > +
> > +	ret = reset_control_deassert(rcrtc->rstc);
> > +	if (ret < 0)
> > +		goto error_peri_clock;
> > +
> > +	rzg2l_du_crtc_setup(rcrtc);
> > +	rcrtc->initialized = true;
> > +
> > +	return 0;
> > +
> > +error_peri_clock:
> > +	clk_disable_unprepare(rcrtc->rzg2l_clocks.pclk);
> > +error_bus_clock:
> > +	clk_disable_unprepare(rcrtc->rzg2l_clocks.aclk);
> > +	return ret;
> > +}
> > +
> > +static void rzg2l_du_crtc_put(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	clk_disable_unprepare(rcrtc->rzg2l_clocks.dclk);
> > +	reset_control_assert(rcrtc->rstc);
> > +	clk_disable_unprepare(rcrtc->rzg2l_clocks.pclk);
> > +	clk_disable_unprepare(rcrtc->rzg2l_clocks.aclk);
> > +
> > +	rcrtc->initialized = false;
> > +}
> > +
> > +static void rzg2l_du_start_stop(struct rzg2l_du_crtc *rcrtc, bool
> start)
> > +{
> > +	struct rzg2l_du_device *rcdu = rcrtc->dev;
> > +
> > +	writel(start ? DU_MCR0_DI_EN : 0, rcdu->mmio + DU_MCR0);
> > +}
> > +
> > +static void rzg2l_du_crtc_start(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	rzg2l_du_start_stop(rcrtc, true);
> > +}
> > +
> > +static void rzg2l_du_crtc_disable_planes(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	struct rzg2l_du_device *rcdu = rcrtc->dev;
> > +	struct drm_crtc *crtc = &rcrtc->crtc;
> > +
> > +	/* Make sure vblank interrupts are enabled. */
> > +	drm_crtc_vblank_get(crtc);
> > +
> > +	if (!wait_event_timeout(rcrtc->vblank_wait, rcrtc->vblank_count ==
> 0,
> > +				msecs_to_jiffies(100)))
> > +		dev_warn(rcdu->dev, "vertical blanking timeout\n");
> > +
> > +	drm_crtc_vblank_put(crtc);
> 
> This while function seems dubious given that vblank_count is never set
> to a non-zero value. I think you need to revisit the CRTC enable/disable
> code to match the needs of your hardware, which seems to be different
> than what the R-Car DU needs.

OK, will drop this function.

> 
> > +}
> > +
> > +static void rzg2l_du_crtc_stop(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	struct drm_crtc *crtc = &rcrtc->crtc;
> > +
> > +	/*
> > +	 * Disable all planes and wait for the change to take effect. This
> is
> > +	 * required as the plane enable registers are updated on vblank,
> and no
> > +	 * vblank will occur once the CRTC is stopped. Disabling planes
> when
> > +	 * starting the CRTC thus wouldn't be enough as it would start
> scanning
> > +	 * out immediately from old frame buffers until the next vblank.
> > +	 *
> > +	 * This increases the CRTC stop delay, especially when multiple
> CRTCs
> > +	 * are stopped in one operation as we now wait for one vblank per
> CRTC.
> > +	 * Whether this can be improved needs to be researched.
> > +	 */
> > +	rzg2l_du_crtc_disable_planes(rcrtc);
> > +
> > +	/*
> > +	 * Disable vertical blanking interrupt reporting. We first need to
> wait
> > +	 * for page flip completion before stopping the CRTC as userspace
> > +	 * expects page flips to eventually complete.
> > +	 */
> > +	rzg2l_du_crtc_wait_page_flip(rcrtc);
> > +	drm_crtc_vblank_off(crtc);
> > +
> > +	/* Disable the VSP compositor. */
> > +	rzg2l_du_vsp_disable(rcrtc);
> > +
> > +	rzg2l_du_start_stop(rcrtc, false);
> > +}
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * CRTC Functions
> > + */
> > +
> > +int __rzg2l_du_crtc_plane_atomic_check(struct drm_plane *plane,
> > +				       struct drm_plane_state *state,
> > +				       const struct rzg2l_du_format_info
> **format)
> 
> This function is only called from rzg2l_du_vsp_plane_atomic_check(), I
> would inline it there.

Agreed.

> 
> > +{
> > +	struct drm_device *dev = plane->dev;
> > +	struct drm_crtc_state *crtc_state;
> > +	int ret;
> > +
> > +	if (!state->crtc) {
> > +		/*
> > +		 * The visible field is not reset by the DRM core but only
> > +		 * updated by drm_plane_helper_check_state(), set it
> manually.
> > +		 */
> > +		state->visible = false;
> > +		*format = NULL;
> > +		return 0;
> > +	}
> > +
> > +	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> > +	if (IS_ERR(crtc_state))
> > +		return PTR_ERR(crtc_state);
> > +
> > +	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> > +						  DRM_PLANE_NO_SCALING,
> > +						  DRM_PLANE_NO_SCALING,
> > +						  true, true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!state->visible) {
> > +		*format = NULL;
> > +		return 0;
> > +	}
> > +
> > +	*format = rzg2l_du_format_info(state->fb->format->format);
> > +	if (*format == NULL) {
> 
> Can this happen, or does the DRM core already checks that the
> framebuffer format is supported by the plane ?

This will make sure the format is as per rzg2l_du_format_info,
Otherwise print unsupported format.

> 
> > +		dev_dbg(dev->dev, "%s: unsupported format %08x\n", __func__,
> > +			state->fb->format->format);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzg2l_du_crtc_atomic_check(struct drm_crtc *crtc,
> > +				      struct drm_atomic_state *state)
> > +{
> > +	struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state,
> > +									  crtc);
> > +	struct rzg2l_du_crtc_state *rstate =
> to_rzg2l_crtc_state(crtc_state);
> > +	struct drm_encoder *encoder;
> > +
> > +	/* Store the routes from the CRTC output to the DU outputs. */
> > +	rstate->outputs = 0;
> > +
> > +	drm_for_each_encoder_mask(encoder, crtc->dev,
> > +				  crtc_state->encoder_mask) {
> > +		struct rzg2l_du_encoder *renc;
> > +
> > +		/* Skip the writeback encoder. */
> > +		if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
> > +			continue;
> > +
> > +		renc = to_rzg2l_encoder(encoder);
> > +		rstate->outputs |= BIT(renc->output);
> > +	}
> 
> Unless I'm mistaken, once you drop dpad0_source, this whole function can
> be dropped too.

I agree. Will drop it.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void rzg2l_du_crtc_atomic_enable(struct drm_crtc *crtc,
> > +					struct drm_atomic_state *state)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	rzg2l_du_crtc_get(rcrtc);
> > +
> > +	rzg2l_du_crtc_start(rcrtc);
> > +}
> > +
> > +static void rzg2l_du_crtc_atomic_disable(struct drm_crtc *crtc,
> > +					 struct drm_atomic_state *state)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	rzg2l_du_crtc_stop(rcrtc);
> > +	rzg2l_du_crtc_put(rcrtc);
> > +
> > +	spin_lock_irq(&crtc->dev->event_lock);
> > +	if (crtc->state->event) {
> > +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > +		crtc->state->event = NULL;
> > +	}
> > +	spin_unlock_irq(&crtc->dev->event_lock);
> > +}
> > +
> > +static void rzg2l_du_crtc_atomic_begin(struct drm_crtc *crtc,
> > +				       struct drm_atomic_state *state)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	WARN_ON(!crtc->state->enable);
> > +
> > +	/*
> > +	 * If a mode set is in progress we can be called with the CRTC
> disabled.
> > +	 * We thus need to first get and setup the CRTC in order to
> configure
> > +	 * planes. We must *not* put the CRTC in .atomic_flush(), as it
> must be
> > +	 * kept awake until the .atomic_enable() call that will follow.
> The get
> > +	 * operation in .atomic_enable() will in that case be a no-op, and
> the
> > +	 * CRTC will be put later in .atomic_disable().
> > +	 *
> > +	 * If a mode set is not in progress the CRTC is enabled, and the
> > +	 * following get call will be a no-op. There is thus no need to
> balance
> > +	 * it in .atomic_flush() either.
> > +	 */
> 
> This should also be reconsidered based on the needs of your hardware,
> given that you don't need to setup planes like in the R-Car DU driver.
> The CRTC handling can most likely be simplified a lot.


OK will drop this function and move the below code to flush.

WARN_ON(!crtc->state->enable);
rzg2l_du_crtc_get(rcrtc); 

> 
> > +	rzg2l_du_crtc_get(rcrtc);
> > +
> > +	rzg2l_du_vsp_atomic_begin(rcrtc);
> > +}
> > +
> > +static void rzg2l_du_crtc_atomic_flush(struct drm_crtc *crtc,
> > +				       struct drm_atomic_state *state)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +	struct drm_device *dev = rcrtc->crtc.dev;
> > +	unsigned long flags;
> > +
> > +	if (crtc->state->event) {
> > +		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > +
> > +		spin_lock_irqsave(&dev->event_lock, flags);
> > +		rcrtc->event = crtc->state->event;
> > +		crtc->state->event = NULL;
> > +		spin_unlock_irqrestore(&dev->event_lock, flags);
> > +	}
> > +
> > +	rzg2l_du_vsp_atomic_flush(rcrtc);
> > +}
> > +
> > +static const struct drm_crtc_helper_funcs crtc_helper_funcs = {
> > +	.atomic_check = rzg2l_du_crtc_atomic_check,
> > +	.atomic_begin = rzg2l_du_crtc_atomic_begin,
> > +	.atomic_flush = rzg2l_du_crtc_atomic_flush,
> > +	.atomic_enable = rzg2l_du_crtc_atomic_enable,
> > +	.atomic_disable = rzg2l_du_crtc_atomic_disable,
> > +};
> > +
> > +static void rzg2l_du_crtc_crc_init(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	const char **sources;
> > +	unsigned int count;
> > +	int i = -1;
> > +
> > +	/* Reserve 1 for "auto" source. */
> > +	count = rcrtc->vsp->num_planes + 1;
> > +
> > +	sources = kmalloc_array(count, sizeof(*sources), GFP_KERNEL);
> > +	if (!sources)
> > +		return;
> > +
> > +	sources[0] = kstrdup("auto", GFP_KERNEL);
> > +	if (!sources[0])
> > +		goto error;
> > +
> > +	for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> > +		struct drm_plane *plane = &rcrtc->vsp->planes[i].plane;
> > +		char name[16];
> > +
> > +		sprintf(name, "plane%u", plane->base.id);
> > +		sources[i + 1] = kstrdup(name, GFP_KERNEL);
> > +		if (!sources[i + 1])
> > +			goto error;
> > +	}
> > +
> > +	rcrtc->sources = sources;
> > +	rcrtc->sources_count = count;
> > +	return;
> > +
> > +error:
> > +	while (i >= 0) {
> > +		kfree(sources[i]);
> > +		i--;
> > +	}
> > +	kfree(sources);
> > +}
> > +
> > +static void rzg2l_du_crtc_crc_cleanup(struct rzg2l_du_crtc *rcrtc)
> > +{
> > +	unsigned int i;
> > +
> > +	if (!rcrtc->sources)
> > +		return;
> > +
> > +	for (i = 0; i < rcrtc->sources_count; i++)
> > +		kfree(rcrtc->sources[i]);
> > +	kfree(rcrtc->sources);
> > +
> > +	rcrtc->sources = NULL;
> > +	rcrtc->sources_count = 0;
> > +}
> > +
> > +static struct drm_crtc_state *
> > +rzg2l_du_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
> > +{
> > +	struct rzg2l_du_crtc_state *state;
> > +	struct rzg2l_du_crtc_state *copy;
> > +
> > +	if (WARN_ON(!crtc->state))
> > +		return NULL;
> > +
> > +	state = to_rzg2l_crtc_state(crtc->state);
> > +	copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
> > +	if (!copy)
> > +		return NULL;
> > +
> > +	__drm_atomic_helper_crtc_duplicate_state(crtc, &copy->state);
> > +
> > +	return &copy->state;
> > +}
> > +
> > +static void rzg2l_du_crtc_atomic_destroy_state(struct drm_crtc *crtc,
> > +					       struct drm_crtc_state *state)
> > +{
> > +	__drm_atomic_helper_crtc_destroy_state(state);
> > +	kfree(to_rzg2l_crtc_state(state));
> > +}
> > +
> > +static void rzg2l_du_crtc_cleanup(struct drm_crtc *crtc)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	rzg2l_du_crtc_crc_cleanup(rcrtc);
> > +
> > +	return drm_crtc_cleanup(crtc);
> > +}
> > +
> > +static void rzg2l_du_crtc_reset(struct drm_crtc *crtc)
> > +{
> > +	struct rzg2l_du_crtc_state *state;
> > +
> > +	if (crtc->state) {
> > +		rzg2l_du_crtc_atomic_destroy_state(crtc, crtc->state);
> > +		crtc->state = NULL;
> > +	}
> > +
> > +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> > +	if (!state)
> > +		return;
> > +
> > +	state->crc.source = VSP1_DU_CRC_NONE;
> > +	state->crc.index = 0;
> > +
> > +	__drm_atomic_helper_crtc_reset(crtc, &state->state);
> > +}
> > +
> > +static int rzg2l_du_crtc_enable_vblank(struct drm_crtc *crtc)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	rcrtc->vblank_enable = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static void rzg2l_du_crtc_disable_vblank(struct drm_crtc *crtc)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	rcrtc->vblank_enable = false;
> > +}
> > +
> > +static int rzg2l_du_crtc_parse_crc_source(struct rzg2l_du_crtc
> *rcrtc,
> > +					  const char *source_name,
> > +					  enum vsp1_du_crc_source *source)
> > +{
> > +	unsigned int index;
> > +	int ret;
> > +
> > +	/*
> > +	 * Parse the source name. Supported values are "plane%u" to
> compute the
> > +	 * CRC on an input plane (%u is the plane ID), and "auto" to
> compute the
> > +	 * CRC on the composer (VSP) output.
> > +	 */
> > +
> > +	if (!source_name) {
> > +		*source = VSP1_DU_CRC_NONE;
> > +		return 0;
> > +	} else if (!strcmp(source_name, "auto")) {
> > +		*source = VSP1_DU_CRC_OUTPUT;
> > +		return 0;
> > +	} else if (strstarts(source_name, "plane")) {
> > +		unsigned int i;
> > +
> > +		*source = VSP1_DU_CRC_PLANE;
> > +
> > +		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> > +			if (index == rcrtc->vsp->planes[i].plane.base.id)
> > +				return i;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int rzg2l_du_crtc_verify_crc_source(struct drm_crtc *crtc,
> > +					   const char *source_name,
> > +					   size_t *values_cnt)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +	enum vsp1_du_crc_source source;
> > +
> > +	if (rzg2l_du_crtc_parse_crc_source(rcrtc, source_name, &source) <
> 0) {
> > +		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
> > +		return -EINVAL;
> > +	}
> > +
> > +	*values_cnt = 1;
> > +	return 0;
> > +}
> > +
> > +static const char *const *
> > +rzg2l_du_crtc_get_crc_sources(struct drm_crtc *crtc, size_t *count)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +
> > +	*count = rcrtc->sources_count;
> > +	return rcrtc->sources;
> > +}
> > +
> > +static int rzg2l_du_crtc_set_crc_source(struct drm_crtc *crtc,
> > +					const char *source_name)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = to_rzg2l_crtc(crtc);
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_atomic_state *state;
> > +	enum vsp1_du_crc_source source;
> > +	unsigned int index;
> > +	int ret;
> > +
> > +	ret = rzg2l_du_crtc_parse_crc_source(rcrtc, source_name, &source);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	index = ret;
> > +
> > +	/* Perform an atomic commit to set the CRC source. */
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +
> > +	state = drm_atomic_state_alloc(crtc->dev);
> > +	if (!state) {
> > +		ret = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	state->acquire_ctx = &ctx;
> > +
> > +retry:
> > +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > +	if (!IS_ERR(crtc_state)) {
> > +		struct rzg2l_du_crtc_state *rcrtc_state;
> > +
> > +		rcrtc_state = to_rzg2l_crtc_state(crtc_state);
> > +		rcrtc_state->crc.source = source;
> > +		rcrtc_state->crc.index = index;
> > +
> > +		ret = drm_atomic_commit(state);
> > +	} else {
> > +		ret = PTR_ERR(crtc_state);
> > +	}
> > +
> > +	if (ret == -EDEADLK) {
> > +		drm_atomic_state_clear(state);
> > +		drm_modeset_backoff(&ctx);
> > +		goto retry;
> > +	}
> > +
> > +	drm_atomic_state_put(state);
> > +
> > +unlock:
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct drm_crtc_funcs crtc_funcs_rz = {
> > +	.reset = rzg2l_du_crtc_reset,
> > +	.destroy = rzg2l_du_crtc_cleanup,
> > +	.set_config = drm_atomic_helper_set_config,
> > +	.page_flip = drm_atomic_helper_page_flip,
> > +	.atomic_duplicate_state = rzg2l_du_crtc_atomic_duplicate_state,
> > +	.atomic_destroy_state = rzg2l_du_crtc_atomic_destroy_state,
> > +	.enable_vblank = rzg2l_du_crtc_enable_vblank,
> > +	.disable_vblank = rzg2l_du_crtc_disable_vblank,
> > +	.set_crc_source = rzg2l_du_crtc_set_crc_source,
> > +	.verify_crc_source = rzg2l_du_crtc_verify_crc_source,
> > +	.get_crc_sources = rzg2l_du_crtc_get_crc_sources,
> > +};
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * Initialization
> > + */
> > +
> > +int rzg2l_du_crtc_create(struct rzg2l_du_device *rcdu)
> > +{
> > +	struct rzg2l_du_crtc *rcrtc = &rcdu->crtcs[0];
> > +	struct drm_crtc *crtc = &rcrtc->crtc;
> > +	struct drm_plane *primary;
> > +	int ret;
> > +
> > +	rcrtc->rstc = devm_reset_control_get_shared(rcdu->dev, NULL);
> > +	if (IS_ERR(rcrtc->rstc)) {
> > +		dev_err(rcdu->dev, "can't get cpg reset\n");
> > +		return PTR_ERR(rcrtc->rstc);
> > +	}
> > +
> > +	rcrtc->rzg2l_clocks.aclk = devm_clk_get(rcdu->dev, "aclk");
> > +	if (IS_ERR(rcrtc->rzg2l_clocks.aclk)) {
> > +		dev_err(rcdu->dev, "no axi clock for DU\n");
> > +		return PTR_ERR(rcrtc->rzg2l_clocks.aclk);
> > +	}
> > +
> > +	rcrtc->rzg2l_clocks.pclk = devm_clk_get(rcdu->dev, "pclk");
> > +	if (IS_ERR(rcrtc->rzg2l_clocks.pclk)) {
> > +		dev_err(rcdu->dev, "no peripheral clock for DU\n");
> > +		return PTR_ERR(rcrtc->rzg2l_clocks.pclk);
> > +	}
> > +
> > +	rcrtc->rzg2l_clocks.dclk = devm_clk_get(rcdu->dev, "vclk");
> > +	if (IS_ERR(rcrtc->rzg2l_clocks.dclk)) {
> > +		dev_err(rcdu->dev, "no video clock for DU\n");
> > +		return PTR_ERR(rcrtc->rzg2l_clocks.dclk);
> > +	}
> > +
> > +	init_waitqueue_head(&rcrtc->flip_wait);
> > +	init_waitqueue_head(&rcrtc->vblank_wait);
> > +	spin_lock_init(&rcrtc->vblank_lock);
> > +
> > +	rcrtc->dev = rcdu;
> > +	rcrtc->index = 0;
> > +
> > +	primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> > +
> > +	ret = drm_crtc_init_with_planes(&rcdu->ddev, crtc, primary, NULL,
> > +					&crtc_funcs_rz, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> > +
> > +	rzg2l_du_crtc_crc_init(rcrtc);
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h
> > new file mode 100644
> > index 000000000000..290b5ea99545
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_crtc.h
> > @@ -0,0 +1,99 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * RZ/G2L Display Unit CRTCs
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + *
> > + * Based on rcar_du_crtc.h
> > + */
> > +
> > +#ifndef __RZG2L_DU_CRTC_H__
> > +#define __RZG2L_DU_CRTC_H__
> > +
> > +#include <linux/mutex.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/wait.h>
> > +
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_writeback.h>
> > +
> > +#include <media/vsp1.h>
> > +
> 
> Missing struct clk. Please go through the headers and add missing
> forward declarations, or drop unneeded ones.

OK will do.

> 
> > +struct reset_control;
> > +struct rzg2l_du_vsp;
> > +struct rzg2l_du_format_info;
> > +
> > +/**
> > + * struct rzg2l_du_crtc - the CRTC, representing a DU superposition
> processor
> > + * @crtc: base DRM CRTC
> > + * @dev: the DU device
> > + * @mmio_offset: offset of the CRTC registers in the DU MMIO block
> > + * @index: CRTC hardware index
> > + * @initialized: whether the CRTC has been initialized and clocks
> enabled
> > + * @vblank_enable: whether vblank events are enabled on this CRTC
> > + * @event: event to post when the pending page flip completes
> > + * @flip_wait: wait queue used to signal page flip completion
> > + * @vblank_lock: protects vblank_wait and vblank_count
> > + * @vblank_wait: wait queue used to signal vertical blanking
> > + * @vblank_count: number of vertical blanking interrupts to wait for
> > + * @vsp: VSP feeding video to this CRTC
> > + * @vsp_pipe: index of the VSP pipeline feeding video to this CRTC
> > + * @rstc: reset controller
> > + * @rzg2l_clocks: the bus, main and video clock
> > + */
> > +struct rzg2l_du_crtc {
> > +	struct drm_crtc crtc;
> > +
> > +	struct rzg2l_du_device *dev;
> > +	unsigned int mmio_offset;
> 
> Not used. Please go through all structure fields and drop the unused
> ones (including both the fully unused fields, and the fields that are
> written but never read).

Agreed.

> 
> > +	unsigned int index;
> > +	bool initialized;
> > +
> > +	bool vblank_enable;
> > +	struct drm_pending_vblank_event *event;
> > +	wait_queue_head_t flip_wait;
> > +
> > +	spinlock_t vblank_lock;
> > +	wait_queue_head_t vblank_wait;
> > +	unsigned int vblank_count;
> > +
> > +	struct rzg2l_du_vsp *vsp;
> > +	unsigned int vsp_pipe;
> > +
> > +	const char *const *sources;
> > +	unsigned int sources_count;
> > +
> > +	struct reset_control *rstc;
> > +	struct {
> > +		struct clk *aclk;
> > +		struct clk *pclk;
> > +		struct clk *dclk;
> > +	} rzg2l_clocks;
> > +};
> > +
> > +#define to_rzg2l_crtc(c)	container_of(c, struct rzg2l_du_crtc,
> crtc)
> 
> A static inline would be better than a macro, it's more type-safe. Same
> for to_rzg2l_crtc_state() and to_rzg2l_encoder().

Will use static inline.
> 
> > +
> > +/**
> > + * struct rzg2l_du_crtc_state - Driver-specific CRTC state
> > + * @state: base DRM CRTC state
> > + * @crc: CRC computation configuration
> > + * @outputs: bitmask of the outputs (enum rzg2l_du_output) driven by
> this CRTC
> > + */
> > +struct rzg2l_du_crtc_state {
> > +	struct drm_crtc_state state;
> > +
> > +	struct vsp1_du_crc_config crc;
> > +	unsigned int outputs;
> > +};
> > +
> > +#define to_rzg2l_crtc_state(s)	container_of(s, struct
> rzg2l_du_crtc_state, state)
> > +
> > +int rzg2l_du_crtc_create(struct rzg2l_du_device *rcdu);
> > +
> > +void rzg2l_du_crtc_finish_page_flip(struct rzg2l_du_crtc *rcrtc);
> > +
> > +int __rzg2l_du_crtc_plane_atomic_check(struct drm_plane *plane,
> > +				       struct drm_plane_state *state,
> > +				       const struct rzg2l_du_format_info
> **format);
> > +
> > +#endif /* __RZG2L_DU_CRTC_H__ */
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > new file mode 100644
> > index 000000000000..0fea1fea837c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.c
> > @@ -0,0 +1,188 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * RZ/G2L Display Unit DRM driver
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + *
> > + * Based on rcar_du_drv.c
> > + */
> > +
> > +#include <linux/clk.h>
> 
> Not needed.

OK.

> 
> > +#include <linux/dma-mapping.h>
> > +#include <linux/io.h>
> 
> Not needed either. Could you check if the other headers are needed ?

OK will do.
> 
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/wait.h>
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_fbdev_generic.h>
> > +#include <drm/drm_gem_dma_helper.h>
> > +#include <drm/drm_managed.h>
> > +#include <drm/drm_probe_helper.h>
> > +
> > +#include "rzg2l_du_drv.h"
> > +#include "rzg2l_du_kms.h"
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * Device Information
> > + */
> > +
> > +static const struct rzg2l_du_device_info rzg2l_du_r9a07g044_info = {
> > +	.channels_mask = BIT(0),
> > +	.routes = {
> > +		[RZG2L_DU_OUTPUT_DSI0] = {
> > +			.possible_crtcs = BIT(0),
> > +			.port = 0,
> > +		},
> > +		[RZG2L_DU_OUTPUT_DPAD0] = {
> > +			.possible_crtcs = BIT(0),
> > +			.port = 1,
> > +		}
> > +	}
> > +};
> > +
> > +static const struct of_device_id rzg2l_du_of_table[] = {
> > +	{ .compatible = "renesas,r9a07g044-du", .data =
> &rzg2l_du_r9a07g044_info },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rzg2l_du_of_table);
> > +
> > +const char *rzg2l_du_output_name(enum rzg2l_du_output output)
> > +{
> > +	static const char * const names[] = {
> > +		[RZG2L_DU_OUTPUT_DSI0] = "DSI0",
> > +		[RZG2L_DU_OUTPUT_DPAD0] = "DPAD0"
> > +	};
> > +
> > +	if (output >= ARRAY_SIZE(names))
> > +		return "UNKNOWN";
> > +
> > +	return names[output];
> > +}
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * DRM operations
> > + */
> > +
> > +DEFINE_DRM_GEM_DMA_FOPS(rzg2l_du_fops);
> > +
> > +static const struct drm_driver rzg2l_du_driver = {
> > +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> > +	.dumb_create		= rzg2l_du_dumb_create,
> > +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> > +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> > +	.gem_prime_import_sg_table = rzg2l_du_gem_prime_import_sg_table,
> > +	.gem_prime_mmap		= drm_gem_prime_mmap,
> > +	.fops			= &rzg2l_du_fops,
> > +	.name			= "rzg2l-du",
> > +	.desc			= "Renesas RZ/G2L Display Unit",
> > +	.date			= "20230410",
> > +	.major			= 1,
> > +	.minor			= 0,
> > +};
> > +
> > +/* ------------------------------------------------------------------
> -----------
> > + * Platform driver
> > + */
> > +
> > +static int rzg2l_du_remove(struct platform_device *pdev)
> > +{
> > +	struct rzg2l_du_device *rcdu = platform_get_drvdata(pdev);
> > +	struct drm_device *ddev = &rcdu->ddev;
> > +
> > +	drm_dev_unregister(ddev);
> > +	drm_atomic_helper_shutdown(ddev);
> > +
> > +	drm_kms_helper_poll_fini(ddev);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rzg2l_du_shutdown(struct platform_device *pdev)
> > +{
> > +	struct rzg2l_du_device *rcdu = platform_get_drvdata(pdev);
> > +
> > +	drm_atomic_helper_shutdown(&rcdu->ddev);
> > +}
> > +
> > +static int rzg2l_du_probe(struct platform_device *pdev)
> > +{
> > +	struct rzg2l_du_device *rcdu;
> > +	int ret;
> > +
> > +	if (drm_firmware_drivers_only())
> > +		return -ENODEV;
> > +
> > +	/* Allocate and initialize the RZ/G2L device structure. */
> > +	rcdu = devm_drm_dev_alloc(&pdev->dev, &rzg2l_du_driver,
> > +				  struct rzg2l_du_device, ddev);
> > +	if (IS_ERR(rcdu))
> > +		return PTR_ERR(rcdu);
> > +
> > +	rcdu->dev = &pdev->dev;
> > +	rcdu->info = of_device_get_match_data(rcdu->dev);
> > +
> > +	platform_set_drvdata(pdev, rcdu);
> > +
> > +	/* I/O resources */
> > +	rcdu->mmio = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(rcdu->mmio))
> > +		return PTR_ERR(rcdu->mmio);
> > +
> > +	/*
> > +	 * When sourcing frames from a VSP the DU doesn't perform any
> memory
> > +	 * access so set the DMA coherent mask to 40 bits to accept all
> buffers.
> > +	 */
> > +	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* DRM/KMS objects */
> > +	ret = rzg2l_du_modeset_init(rcdu);
> > +	if (ret < 0) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(&pdev->dev,
> > +				"failed to initialize DRM/KMS (%d)\n", ret);
> 
> Use dev_err_probe()

As per your patch [1], I guess it is not required

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230530153251.22302-1-laurent.pinchart+renesas@xxxxxxxxxxxxxxxx/

> 
> > +		goto error;
> > +	}
> > +
> > +	/*
> > +	 * Register the DRM device with the core and the connectors with
> > +	 * sysfs.
> > +	 */
> > +	ret = drm_dev_register(&rcdu->ddev, 0);
> > +	if (ret)
> > +		goto error;
> > +
> > +	DRM_INFO("Device %s probed\n", dev_name(&pdev->dev));
> 
> Use drm_info().

Agreed.

> 
> > +
> > +	drm_fbdev_generic_setup(&rcdu->ddev, 32);
> > +
> > +	return 0;
> > +
> > +error:
> > +	drm_kms_helper_poll_fini(&rcdu->ddev);
> > +	return ret;
> > +}
> > +
> > +static struct platform_driver rzg2l_du_platform_driver = {
> > +	.probe		= rzg2l_du_probe,
> > +	.remove		= rzg2l_du_remove,
> > +	.shutdown	= rzg2l_du_shutdown,
> > +	.driver		= {
> > +		.name	= "rzg2l-du",
> > +		.of_match_table = rzg2l_du_of_table,
> > +	},
> > +};
> > +
> > +module_platform_driver(rzg2l_du_platform_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L Display Unit DRM Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > new file mode 100644
> > index 000000000000..3b84e91aa64a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_du_drv.h
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * RZ/G2L Display Unit DRM driver
> > + *
> > + * Copyright (C) 2023 Renesas Electronics Corporation
> > + *
> > + * Based on rcar_du_drv.h
> > + */
> > +
> > +#ifndef __RZG2L_DU_DRV_H__
> > +#define __RZG2L_DU_DRV_H__
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/wait.h>
> 
> Not needed.

OK.

> 
> > +
> > +#include <drm/drm_device.h>
> > +
> > +#include "rzg2l_du_crtc.h"
> > +#include "rzg2l_du_vsp.h"
> > +
> > +struct clk;
> 
> Not used in this header.

Will remove.

> 
> > +struct device;
> > +struct drm_bridge;
> > +struct drm_property;
> > +struct rzg2l_du_device;
> 
> Not needed.

Ooops. Will take out.

> 
> > +
> > +enum rzg2l_du_output {
> > +	RZG2L_DU_OUTPUT_DSI0,
> > +	RZG2L_DU_OUTPUT_DPAD0,
> > +	RZG2L_DU_OUTPUT_MAX,
> > +};
> > +
> > +/*
> > + * struct rzg2l_du_output_routing - Output routing specification
> > + * @possible_crtcs: bitmask of possible CRTCs for the output
> > + * @port: device tree port number corresponding to this output route
> > + *
> > + * The DU has 2 possible outputs (DPAD0, DSI0). Output routing data
> > + * specify the valid SoC outputs, which CRTCs can drive the output,
> and the type
> > + * of in-SoC encoder for the output.
> > + */
> > +struct rzg2l_du_output_routing {
> > +	unsigned int possible_crtcs;
> > +	unsigned int port;
> > +};
> > +
> > +/*
> > + * struct rzg2l_du_device_info - DU model-specific information
> > + * @channels_mask: bit mask of available DU channels
> > + * @routes: array of CRTC to output routes, indexed by output
> (RZG2L_DU_OUTPUT_*)
> > + */
> > +struct rzg2l_du_device_info {
> > +	unsigned int channels_mask;
> > +	struct rzg2l_du_output_routing routes[RZG2L_DU_OUTPUT_MAX];
> > +};
> 
> The driver supports a single SoC, with two outputs, connected to the
> same DU channel. Do you really need to copy the rzg2l_du_device_info
> abstraction from the rcar-du driver, or could you simplify the code ?

After adding basic support, as an optimization
will simplify the code later. Hope it is ok for you??


> > +
> > +#define RZG2L_DU_MAX_CRTCS		1
> > +#define RZG2L_DU_MAX_VSPS		1
> > +#define RZG2L_DU_MAX_DSI		1
> > +
> > +struct rzg2l_du_device {
> > +	struct device *dev;
> > +	const struct rzg2l_du_device_info *info;
> > +
> > +	void __iomem *mmio;
> > +
> > +	struct drm_device ddev;
> > +
> > +	struct rzg2l_du_crtc crtcs[RZG2L_DU_MAX_CRTCS];
> > +	unsigned int num_crtcs;
> > +
> > +	struct rzg2l_du_vsp vsps[RZG2L_DU_MAX_VSPS];
> > +	struct drm_bridge *dsi[RZG2L_DU_MAX_DSI];
> 
> This is written but never read. You can drop the field.

Agreed.

> 
> > +
> > +	struct {
> > +		struct drm_property *colorkey;
> > +	} props;
> > +
> > +	unsigned int dpad0_source;
> 
> This is written but never read. You can drop the field.

Agreed. Will add later when we add support for DPI.

Cheers,
Biju




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux