drm/tilcdc: Atomic modeset support

Add tilcdc_crtc_mode_set_nofb(). The mode_set_nofb() semantics do not fit well to LCDC, because of the mandatory framebuffer. However, when the primary plane is required in the check phase, it and the framebuffer can be found from the atomic state struct.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 172 +++++++++++++++++++++++++++++++++++
 1 file changed, 172 insertions(+)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +172 -0

index 919c901..35f5682 100644
@@ -310,6 +310,177 @@ static void tilcdc_crtc_commit(struct drm_crtc *crtc)
 	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
 }
 
+static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
+{
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	const struct tilcdc_panel_info *info = tilcdc_crtc->info;
+	uint32_t reg, hbp, hfp, hsw, vbp, vfp, vsw;
+	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+	struct drm_framebuffer *fb = crtc->primary->state->fb;
+
+	if (WARN_ON(!info))
+		return;
+
+	if (WARN_ON(!fb))
+		return;
+
+	pm_runtime_get_sync(dev->dev);
+
+	/* Configure the Burst Size and fifo threshold of DMA: */
+	reg = tilcdc_read(dev, LCDC_DMA_CTRL_REG) & ~0x00000770;
+	switch (info->dma_burst_sz) {
+	case 1:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_1);
+		break;
+	case 2:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_2);
+		break;
+	case 4:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_4);
+		break;
+	case 8:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_8);
+		break;
+	case 16:
+		reg |= LCDC_DMA_BURST_SIZE(LCDC_DMA_BURST_16);
+		break;
+	default:
+		dev_err(dev->dev, "invalid burst size\n");
+		return;
+	}
+	reg |= (info->fifo_th << 8);
+	tilcdc_write(dev, LCDC_DMA_CTRL_REG, reg);
+
+	/* Configure timings: */
+	hbp = mode->htotal - mode->hsync_end;
+	hfp = mode->hsync_start - mode->hdisplay;
+	hsw = mode->hsync_end - mode->hsync_start;
+	vbp = mode->vtotal - mode->vsync_end;
+	vfp = mode->vsync_start - mode->vdisplay;
+	vsw = mode->vsync_end - mode->vsync_start;
+
+	DBG("%dx%d, hbp=%u, hfp=%u, hsw=%u, vbp=%u, vfp=%u, vsw=%u",
+	    mode->hdisplay, mode->vdisplay, hbp, hfp, hsw, vbp, vfp, vsw);
+
+	/* Set AC Bias Period and Number of Transitions per Interrupt: */
+	reg = tilcdc_read(dev, LCDC_RASTER_TIMING_2_REG) & ~0x000fff00;
+	reg |= LCDC_AC_BIAS_FREQUENCY(info->ac_bias) |
+		LCDC_AC_BIAS_TRANSITIONS_PER_INT(info->ac_bias_intrpt);
+
+	/*
+	 * subtract one from hfp, hbp, hsw because the hardware uses
+	 * a value of 0 as 1
+	 */
+	if (priv->rev == 2) {
+		/* clear bits we're going to set */
+		reg &= ~0x78000033;
+		reg |= ((hfp-1) & 0x300) >> 8;
+		reg |= ((hbp-1) & 0x300) >> 4;
+		reg |= ((hsw-1) & 0x3c0) << 21;
+	}
+	tilcdc_write(dev, LCDC_RASTER_TIMING_2_REG, reg);
+
+	reg = (((mode->hdisplay >> 4) - 1) << 4) |
+		(((hbp-1) & 0xff) << 24) |
+		(((hfp-1) & 0xff) << 16) |
+		(((hsw-1) & 0x3f) << 10);
+	if (priv->rev == 2)
+		reg |= (((mode->hdisplay >> 4) - 1) & 0x40) >> 3;
+	tilcdc_write(dev, LCDC_RASTER_TIMING_0_REG, reg);
+
+	reg = ((mode->vdisplay - 1) & 0x3ff) |
+		((vbp & 0xff) << 24) |
+		((vfp & 0xff) << 16) |
+		(((vsw-1) & 0x3f) << 10);
+	tilcdc_write(dev, LCDC_RASTER_TIMING_1_REG, reg);
+
+	/*
+	 * be sure to set Bit 10 for the V2 LCDC controller,
+	 * otherwise limited to 1024 pixels width, stopping
+	 * 1920x1080 being supported.
+	 */
+	if (priv->rev == 2) {
+		if ((mode->vdisplay - 1) & 0x400) {
+			tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG,
+				LCDC_LPP_B10);
+		} else {
+			tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG,
+				LCDC_LPP_B10);
+		}
+	}
+
+	/* Configure display type: */
+	reg = tilcdc_read(dev, LCDC_RASTER_CTRL_REG) &
+		~(LCDC_TFT_MODE | LCDC_MONO_8BIT_MODE | LCDC_MONOCHROME_MODE |
+		  LCDC_V2_TFT_24BPP_MODE | LCDC_V2_TFT_24BPP_UNPACK |
+		  0x000ff000 /* Palette Loading Delay bits */);
+	reg |= LCDC_TFT_MODE; /* no monochrome/passive support */
+	if (info->tft_alt_mode)
+		reg |= LCDC_TFT_ALT_ENABLE;
+	if (priv->rev == 2) {
+		unsigned int depth, bpp;
+
+		drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
+		switch (bpp) {
+		case 16:
+			break;
+		case 32:
+			reg |= LCDC_V2_TFT_24BPP_UNPACK;
+			/* fallthrough */
+		case 24:
+			reg |= LCDC_V2_TFT_24BPP_MODE;
+			break;
+		default:
+			dev_err(dev->dev, "invalid pixel format\n");
+			return;
+		}
+	}
+	reg |= info->fdd < 12;
+	tilcdc_write(dev, LCDC_RASTER_CTRL_REG, reg);
+
+	if (info->invert_pxl_clk)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_PIXEL_CLOCK);
+
+	if (info->sync_ctrl)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_CTRL);
+
+	if (info->sync_edge)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_SYNC_EDGE);
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_HSYNC);
+
+	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+		tilcdc_set(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_TIMING_2_REG, LCDC_INVERT_VSYNC);
+
+	if (info->raster_order)
+		tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
+	else
+		tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
+
+	drm_framebuffer_reference(fb);
+
+	set_scanout(crtc, fb);
+
+	tilcdc_crtc_update_clk(crtc);
+
+	pm_runtime_put_sync(dev->dev);
+
+	crtc->hwmode = crtc->state->adjusted_mode;
+}
+
 static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
 		struct drm_display_mode *mode,
 		struct drm_display_mode *adjusted_mode,

@@ -526,6 +697,7 @@ static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
 		.commit         = tilcdc_crtc_commit,
 		.mode_set       = tilcdc_crtc_mode_set,
 		.mode_set_base  = tilcdc_crtc_mode_set_base,
+		.mode_set_nofb	= tilcdc_crtc_mode_set_nofb,
 };
 
 int tilcdc_crtc_max_width(struct drm_crtc *crtc)

Signature · 1 line

1.9.1

Set DRIVER_ATOMIC and use atomic helpers and rename commit and prepare crtc helpers to enable and disable. This makes the final jump to mode setting, but there is lot of obsolete code to clean up.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 +++++++++++-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +9 -5

index 69045d8..e8e309e 100644
@@ -17,6 +17,7 @@
 
 #include "drm_flip_work.h"
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"

@@ -300,12 +301,12 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
-static void tilcdc_crtc_prepare(struct drm_crtc *crtc)
+static void tilcdc_crtc_disable(struct drm_crtc *crtc)
 {
 	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 }
 
-static void tilcdc_crtc_commit(struct drm_crtc *crtc)
+static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 {
 	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
 }

@@ -713,9 +714,12 @@ static int tilcdc_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
 }
 
 static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
-		.destroy        = tilcdc_crtc_destroy,
-		.set_config     = drm_crtc_helper_set_config,
-		.page_flip      = tilcdc_crtc_page_flip,
+	.destroy        = tilcdc_crtc_destroy,
+	.set_config     = drm_atomic_helper_set_config,
+	.page_flip      = drm_atomic_helper_page_flip,
+	.reset		= drm_atomic_helper_crtc_reset,
+	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
 static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +2 -0

 		.mode_set       = tilcdc_crtc_mode_set,
 		.mode_set_base  = tilcdc_crtc_mode_set_base,
+		.enable		= tilcdc_crtc_enable,
+		.disable	= tilcdc_crtc_disable,
 		.atomic_check	= tilcdc_crtc_atomic_check,
 		.mode_set_nofb	= tilcdc_crtc_mode_set_nofb,
 };

drivers/gpu/drm/tilcdc/tilcdc_drv.c +0 -0

index dc0d1e9..6569d3a 100644

The LCDC in its simplicity does not fit too well into DRM atomic modeset abstractions. I wonder if I am doing the right thing in implementing the dummy primary plane and in implementing mode_set_nofb() crtc helper when the crtc actually needs the framebuffer to be there when configuring it. See individual patch descriptions for details. There is still lot of room for cleaning up but I would first like to know if I am moving at all to the right direction.

Hi Jyri,

Quoted 8 lines · Jyri Sarha

The LCDC in its simplicity does not fit too well into DRM atomic modeset abstractions. I wonder if I am doing the right thing in implementing the dummy primary plane and in implementing mode_set_nofb() crtc helper when the crtc actually needs the framebuffer to be there when configuring it. See individual patch descriptions for details. There is still lot of room for cleaning up but I would first like to know if I am moving at all to the right direction.

I'm no expert on atomic modesetting, but here are my comments/questions:

You add drm_crtc_vblank_off() to crtc_destroy() callback. Why is that? I think you should call drm_crtc_vblank_on() in crtc_enable(), and vblank_off in disable.

You remove the setting of tilcdc_crtc_helper_funcs.dpms, but leave the tilcdc_crtc_dpms, which you use elsewhere. I think all dpms stuff should be removed from crtc, as it's all either "enable" or "disable". Git grep shows that in omapdrm, there's just one reference to dpms, in omap_connector.c: .dpms = drm_atomic_helper_connector_dpms

It's not clear to me if a (primary) plane is required with atomic universal planes and modesetting or not... I guess it is, when thinking of how e.g. a framebuffer is configured to be shown on a screen when using the atomic modesetting uapi.

But if it is required, it makes me wonder, are there other HWs out there without any planes? The dummy plane implementation you added is not complex, but is it something that should be implemented with DRM helper funcs?

Quoted 4 lines

Quoted 4 lines

It's not clear to me if a (primary) plane is required with atomic universal planes and modesetting or not... I guess it is, when thinking of how e.g. a framebuffer is configured to be shown on a screen when using the atomic modesetting uapi.

You need a primary plane, but atomic doesn't require that it's enabled. Which this simple display controller probably wont like, so seems like this implementation of a primary plane is a bit too simple. You also need

a real plane for the cursor, if you want to support that with atomic.

Quoted 4 lines

Quoted 4 lines

But if it is required, it makes me wonder, are there other HWs out there without any planes? The dummy plane implementation you added is not complex, but is it something that should be implemented with DRM helper funcs?

There's a drm_simple_display_pipe floating around which seems perfectly suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc and 1 encoder maybe linking to different connectors. And it takes care of all the small bits for you, with a grand total of 5 callbacks, all of them optional.

Might indeed be useful to rebase tilcdc on top of that, should be possible to nuke piles of code.

Quoted 7 lines · Daniel Vetter

Quoted 4 lines

Quoted 4 lines

It's not clear to me if a (primary) plane is required with atomic universal planes and modesetting or not... I guess it is, when thinking of how e.g. a framebuffer is configured to be shown on a screen when using the atomic modesetting uapi.

You need a primary plane, but atomic doesn't require that it's enabled. Which this simple display controller probably wont like, so seems like this implementation of a primary plane is a bit too simple. You also need

So I do what I can, by checking in crtc check that the plane is part of the new state. What more should I do?g

Quoted 2 lines

a real plane for the cursor, if you want to support that with atomic.

Well, there is no such thing in LCDC.

Quoted 10 lines

Quoted 4 lines

Quoted 4 lines

But if it is required, it makes me wonder, are there other HWs out there without any planes? The dummy plane implementation you added is not complex, but is it something that should be implemented with DRM helper funcs?

There's a drm_simple_display_pipe floating around which seems perfectly suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc and 1 encoder maybe linking to different connectors. And it takes care of all the small bits for you, with a grand total of 5 callbacks, all of them optional.

Quoted 2 lines

Might indeed be useful to rebase tilcdc on top of that, should be possible to nuke piles of code.

Looks interesting. Does it look like it is getting ready to be merged soon?

Quoted 10 lines

Quoted 10 lines

There's a drm_simple_display_pipe floating around which seems perfectly

Quoted 7 lines

Quoted 7 lines

suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc and 1 encoder maybe linking to different connectors. And it takes care of all the small bits for you, with a grand total of 5 callbacks, all of them optional.

Might indeed be useful to rebase tilcdc on top of that, should be possible to nuke piles of code.

Quoted 1 line

Quoted 1 line

Looks interesting. Does it look like it is getting ready to be merged soon?

Should be landind soon, yes. Probably not for 4.7, that's closed now, but I can still pick it up into drm-misc right away when it's ready. Open review comments are all just small things, you could pick the latest version to start prototyping a conversion and there shouldn't be any surprises when you rebase onto the merged version.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

There's a drm_simple_display_pipe floating around which seems perfectly

Quoted 7 lines

Quoted 7 lines

suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc and 1 encoder maybe linking to different connectors. And it takes care of all the small bits for you, with a grand total of 5 callbacks, all of them optional.

Might indeed be useful to rebase tilcdc on top of that, should be possible to nuke piles of code.

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Looks interesting. Does it look like it is getting ready to be merged soon?

Should be landind soon, yes. Probably not for 4.7, that's closed now, but I can still pick it up into drm-misc right away when it's ready. Open review comments are all just small things, you could pick the latest version to start prototyping a conversion and there shouldn't be any surprises when you rebase onto the merged version.

Hmmm, too bad it wants to own its encoder. LCDC on Beaglebone-Black needs the componentized external tda998x driver. So at least in its current form the drm_simple_display_pipe wont work for tilcdc.

It may not be too big a job to add an external encoder support, but would it complicate currently so nice and simple driver structure too much?

Jyri

Den 10.05.2016 11:14, skrev Jyri Sarha:

Quoted 31 lines · Daniel Vetter

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

There's a drm_simple_display_pipe floating around which seems perfectly

Quoted 12 lines

Quoted 12 lines

suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc
and 1 encoder maybe linking to different connectors. And it takes care of
all the small bits for you, with a grand total of 5 callbacks, all of them
optional.

Might indeed be useful to rebase tilcdc on top of that, should be possible
to nuke piles of code.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Looks interesting. Does it look like it is getting ready to be merged soon?

Should be landind soon, yes. Probably not for 4.7, that's closed now, but I can still pick it up into drm-misc right away when it's ready. Open review comments are all just small things, you could pick the latest version to start prototyping a conversion and there shouldn't be any surprises when you rebase onto the merged version.

Hmmm, too bad it wants to own its encoder. LCDC on Beaglebone-Black needs the componentized external tda998x driver. So at least in its current form the drm_simple_display_pipe wont work for tilcdc.

Quoted 3 lines

It may not be too big a job to add an external encoder support, but would it complicate currently so nice and simple driver structure too much?

How about we add an argument for encoder and if it is NULL, then the no-op encoder is used:

Quoted 29 lines · Noralf Trønnes

Den 10.05.2016 11:14, skrev Jyri Sarha:

Quoted 27 lines · Daniel Vetter

Quoted 12 lines

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

Quoted 5 lines

There's a drm_simple_display_pipe floating around which seems perfectly suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc and 1 encoder maybe linking to different connectors. And it takes care of all the small bits for you, with a grand total of 5 callbacks, all of them optional.

Might indeed be useful to rebase tilcdc on top of that, should be possible to nuke piles of code.

Looks interesting. Does it look like it is getting ready to be merged soon?

Quoted 6 lines

Should be landind soon, yes. Probably not for 4.7, that's closed now, but
I can still pick it up into drm-misc right away when it's ready. Open review comments are all just small things, you could pick the latest version to start prototyping a conversion and there shouldn't be any surprises when you rebase onto the merged version.

Hmmm, too bad it wants to own its encoder. LCDC on Beaglebone-Black needs the componentized external tda998x driver. So at least in its current form the drm_simple_display_pipe wont work for tilcdc.

It may not be too big a job to add an external encoder support, but would it complicate currently so nice and simple driver structure too much?

Quoted 2 lines

How about we add an argument for encoder and if it is NULL, then the no-op encoder is used:

Hm, my idea with external transcoders was to pull them in as a drm_bridge. That's of course more work, and we already have a proliferation of different transcoder driver standards in drm unfortunately (there's drm_bridge, but als to drm_encoder_slave).

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Hmmm, too bad it wants to own its encoder. LCDC on Beaglebone-Black needs the componentized external tda998x driver. So at least in its current form the drm_simple_display_pipe wont work for tilcdc.

It may not be too big a job to add an external encoder support, but would it complicate currently so nice and simple driver structure too much?

How about we add an argument for encoder and if it is NULL, then the no-op encoder is used:

Quoted 4 lines

Hm, my idea with external transcoders was to pull them in as a drm_bridge. That's of course more work, and we already have a proliferation of different transcoder driver standards in drm unfortunately (there's drm_bridge, but als to drm_encoder_slave).

drm_encoder_slave has to go. As a first step the adv7511 driver is being converted to a drm_bridge by Archit. Any volunteer for the three other drivers ? We also need to clearly state that new drivers must use drm_bridge.

Quoted 30 lines

Den 10.05.2016 11:14, skrev Jyri Sarha:

Quoted 28 lines · Daniel Vetter

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

There's a drm_simple_display_pipe floating around which seems perfectly

Quoted 11 lines

Quoted 11 lines

suited to tilcdc. It's meant for the case where you have 1 plane, 1 crtc
and 1 encoder maybe linking to different connectors. And it takes care of
all the small bits for you, with a grand total of 5 callbacks, all of them
optional.

Might indeed be useful to rebase tilcdc on top of that, should be possible
to nuke piles of code.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Looks interesting. Does it look like it is getting ready to be merged soon?

Should be landind soon, yes. Probably not for 4.7, that's closed now, but I can still pick it up into drm-misc right away when it's ready. Open review comments are all just small things, you could pick the latest version to start prototyping a conversion and there shouldn't be any surprises when you rebase onto the merged version.

Hmmm, too bad it wants to own its encoder. LCDC on Beaglebone-Black needs the componentized external tda998x driver. So at least in its current form the drm_simple_display_pipe wont work for tilcdc.

It may not be too big a job to add an external encoder support, but would it complicate currently so nice and simple driver structure too much?

Quoted 2 lines

How about we add an argument for encoder and if it is NULL, then the no-op encoder is used:

Hm, my idea with external transcoders was to pull them in as a drm_bridge. That's of course more work, and we already have a proliferation of different transcoder driver standards in drm unfortunately (there's drm_bridge, but als to drm_encoder_slave).

Quoted 4 lines

Hm, my idea with external transcoders was to pull them in as a drm_bridge. That's of course more work, and we already have a proliferation of different transcoder driver standards in drm unfortunately (there's drm_bridge, but als to drm_encoder_slave).

Does this mean that you want the drm_bridge_*() functions in disable_outputs(), crtc_set_mode() and drm_atomic_helper_commit_modeset_enables() to run for drm_simple_display_pipe?

Because "drm: Make drm_encoder_helper_funcs optional" skips those bridge functions if encoder->helper_private is not set (I found this pattern in mode_fixup() which skips drm_bridge_mode_fixup() on !funcs). So if that's the case, I have to add an empty drm_encoder_helper_funcs to the
simple pipe.

Quoted 7 lines · Daniel Vetter

Quoted 4 lines

Quoted 4 lines

It's not clear to me if a (primary) plane is required with atomic universal planes and modesetting or not... I guess it is, when thinking of how e.g. a framebuffer is configured to be shown on a screen when using the atomic modesetting uapi.

You need a primary plane, but atomic doesn't require that it's enabled. Which this simple display controller probably wont like, so seems like this implementation of a primary plane is a bit too simple. You also need

So I do what I can, by checking in crtc check that the plane is part of the new state. What more should I do?g

Hi Jyri,

Quoted 8 lines · Jyri Sarha

The LCDC in its simplicity does not fit too well into DRM atomic modeset abstractions. I wonder if I am doing the right thing in implementing the dummy primary plane and in implementing mode_set_nofb() crtc helper when the crtc actually needs the framebuffer to be there when configuring it. See individual patch descriptions for details. There is still lot of room for cleaning up but I would first like to know if I am moving at all to the right direction.

I'm no expert on atomic modesetting, but here are my comments/questions:

You add drm_crtc_vblank_off() to crtc_destroy() callback. Why is that? I think you should call drm_crtc_vblank_on() in crtc_enable(), and vblank_off in disable.

You remove the setting of tilcdc_crtc_helper_funcs.dpms, but leave the tilcdc_crtc_dpms, which you use elsewhere. I think all dpms stuff should be removed from crtc, as it's all either "enable" or "disable". Git grep shows that in omapdrm, there's just one reference to dpms, in omap_connector.c: .dpms = drm_atomic_helper_connector_dpms

It's not clear to me if a (primary) plane is required with atomic universal planes and modesetting or not... I guess it is, when thinking of how e.g. a framebuffer is configured to be shown on a screen when using the atomic modesetting uapi.

But if it is required, it makes me wonder, are there other HWs out there without any planes? The dummy plane implementation you added is not complex, but is it something that should be implemented with DRM helper funcs?

Tomi