[PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[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.

Jyri Sarha (11):
  drm/tilcdc: Make tilcdc_crtc_page_flip() public
  drm/tilcdc: Add dummy primary plane implementation
  drm/tilcdc: Initialize dummy primary plane from crtc init
  drm/tilcdc: Add tilcdc_crtc_mode_set_nofb()
  drm/tilcdc: Add tilcdc_crtc_atomic_check()
  drm/tilcdc: Add atomic mode config funcs
  drm/tilcdc: Add drm_mode_config_reset() call to tilcdc_load()
  drm/tilcdc: Call drm_crtc_vblank_off() in tilcdc_crtc_destroy()
  drm/tilcdc: Set DRIVER_ATOMIC and use atomic crtc helpers
  drm/tilcdc: Remove obsolete crtc helper functions
  drm/tilcdc: Remove tilcdc_verify_fb()

 drivers/gpu/drm/tilcdc/Makefile       |   1 +
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c  | 142 +++++++++++++++-------------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c   |  52 ++++++++++++-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h   |   6 ++
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 122 +++++++++++++++++++++++++++++
 5 files changed, 244 insertions(+), 79 deletions(-)
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_plane.c

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 01/11] drm/tilcdc: Make tilcdc_crtc_page_flip() public

[Jyri Sarha]

Make tilcdc_crtc_page_flip() public for dummy plane implementation to use.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 6dce763..78466ed 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -184,7 +184,7 @@ static int tilcdc_verify_fb(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 	return 0;
 }
 
-static int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
+int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
 		struct drm_framebuffer *fb,
 		struct drm_pending_vblank_event *event,
 		uint32_t page_flip_flags)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index c1de18b..8707ae6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -172,5 +172,9 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
 int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
 int tilcdc_crtc_max_width(struct drm_crtc *crtc);
 void tilcdc_crtc_dpms(struct drm_crtc *crtc, int mode);
+int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
+		struct drm_framebuffer *fb,
+		struct drm_pending_vblank_event *event,
+		uint32_t page_flip_flags);
 
 #endif /* __TILCDC_DRV_H__ */
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 02/11] drm/tilcdc: Add dummy primary plane implementation

[Jyri Sarha]

Add dummy primary plane implementation. LCDC does not really have
planes, only simple framebuffer that is mandatory. This primary plane
implementation has the necessary checks for implementing simple
framebuffer trough DRM plane abstraction. For setting the actual
framebuffer the implementation relies on a CRTC side function.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/Makefile       |   1 +
 drivers/gpu/drm/tilcdc/tilcdc_drv.h   |   2 +
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 122 ++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+)
 create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_plane.c

diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
index deeca48..6f67517 100644
--- a/drivers/gpu/drm/tilcdc/Makefile
+++ b/drivers/gpu/drm/tilcdc/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_DRM_TILCDC_SLAVE_COMPAT) += tilcdc_slave_compat.o \
 					 tilcdc_slave_compat.dtb.o
 
 tilcdc-y := \
+	tilcdc_plane.o \
 	tilcdc_crtc.o \
 	tilcdc_tfp410.o \
 	tilcdc_panel.o \
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 8707ae6..725d0b6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -177,4 +177,6 @@ int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
 		struct drm_pending_vblank_event *event,
 		uint32_t page_flip_flags);
 
+int tilcdc_plane_init(struct drm_device *dev, struct drm_plane *plane);
+
 #endif /* __TILCDC_DRV_H__ */
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
new file mode 100644
index 0000000..c5d069e
--- /dev/null
+++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) 2015 Texas Instruments
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <drm/drmP.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_atomic_helper.h>
+#include <uapi/drm/drm_fourcc.h>
+
+#include "tilcdc_drv.h"
+
+static const u32 tilcdc_formats[] = { DRM_FORMAT_RGB565,
+				      DRM_FORMAT_RGB888,
+				      DRM_FORMAT_XRGB8888 };
+
+static struct drm_plane_funcs tilcdc_plane_funcs = {
+	.update_plane	= drm_atomic_helper_update_plane,
+	.disable_plane	= drm_atomic_helper_disable_plane,
+	.destroy	= drm_plane_cleanup,
+	.set_property	= drm_atomic_helper_plane_set_property,
+	.reset		= drm_atomic_helper_plane_reset,
+	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static int tilcdc_plane_atomic_check(struct drm_plane *plane,
+				     struct drm_plane_state *state)
+{
+	unsigned int depth, bpp;
+	struct drm_crtc_state *crtc_state;
+
+	if (!state->crtc)
+		return 0;
+
+	if (WARN_ON(!state->fb))
+		return -EINVAL;
+
+	if (state->crtc_x || state->crtc_y) {
+		dev_err(plane->dev->dev, "%s: crtc position must be zero.",
+			__func__);
+		return -EINVAL;
+	}
+
+	crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
+
+	if (crtc_state->mode.hdisplay != state->crtc_w ||
+	    crtc_state->mode.vdisplay != state->crtc_h) {
+		dev_err(plane->dev->dev,
+			"%s: Size must match mode (%dx%d == %dx%d)", __func__,
+			crtc_state->mode.hdisplay, crtc_state->mode.vdisplay,
+			state->crtc_w, state->crtc_h);
+		return -EINVAL;
+	}
+
+	drm_fb_get_bpp_depth(state->fb->pixel_format, &depth, &bpp);
+	if (state->fb->pitches[0] != crtc_state->mode.hdisplay * bpp / 8) {
+		dev_err(plane->dev->dev,
+			"Invalid pitch: fb and crtc widths must be the same");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void tilcdc_plane_atomic_update(struct drm_plane *plane,
+				       struct drm_plane_state *old_state)
+{
+	struct drm_plane_state *state = plane->state;
+
+	if (!state->crtc)
+		return;
+
+	if (WARN_ON(!state->fb || !state->crtc->state))
+		return;
+
+	tilcdc_crtc_page_flip(state->crtc,
+			      state->fb,
+			      state->crtc->state->event,
+			      0);
+}
+
+static const struct drm_plane_helper_funcs plane_helper_funcs = {
+	.atomic_check = tilcdc_plane_atomic_check,
+	.atomic_update = tilcdc_plane_atomic_update,
+};
+
+int tilcdc_plane_init(struct drm_device *dev,
+		      struct drm_plane *plane)
+{
+	int ret;
+
+	ret = drm_plane_init(dev, plane, 1,
+			     &tilcdc_plane_funcs,
+			     tilcdc_formats,
+			     ARRAY_SIZE(tilcdc_formats),
+			     true);
+	if (ret) {
+		dev_err(dev->dev, "Failed to initialize plane: %d\n", ret);
+		return ret;
+	}
+
+	drm_plane_helper_add(plane, &plane_helper_funcs);
+
+	return 0;
+}
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 03/11] drm/tilcdc: Initialize dummy primary plane from crtc init

[Jyri Sarha]

Initialize dummy primary plane from crtc init.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 78466ed..919c901 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -26,6 +26,7 @@
 struct tilcdc_crtc {
 	struct drm_crtc base;
 
+	struct drm_plane primary;
 	const struct tilcdc_panel_info *info;
 	struct drm_pending_vblank_event *event;
 	int dpms;
@@ -793,6 +794,10 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 
 	crtc = &tilcdc_crtc->base;
 
+	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
+	if (ret < 0)
+		goto fail;
+
 	tilcdc_crtc->dpms = DRM_MODE_DPMS_OFF;
 	init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
 
@@ -802,7 +807,11 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 	spin_lock_init(&tilcdc_crtc->irq_lock);
 	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
 
-	ret = drm_crtc_init(dev, crtc, &tilcdc_crtc_funcs);
+	ret = drm_crtc_init_with_planes(dev, crtc,
+					&tilcdc_crtc->primary,
+					NULL,
+					&tilcdc_crtc_funcs,
+					"tilcdc crtc");
 	if (ret < 0)
 		goto fail;
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 04/11] drm/tilcdc: Add tilcdc_crtc_mode_set_nofb()

[Jyri Sarha]

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 <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 172 +++++++++++++++++++++++++++++++++++
 1 file changed, 172 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 919c901..35f5682 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -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)
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 04/11] drm/tilcdc: Add tilcdc_crtc_mode_set_nofb()

[Daniel Vetter]

On Mon, Apr 11, 2016 at 07:46:31PM +0300, Jyri Sarha wrote:
> 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 <jsarha@ti.com>

Yeah, if your hw always requires a primary plane then 2 bits needed: a)
make sure in the crtc's atomic_check it's there and set b) just use the
hooks whoever you want to. Atomic allows planes to be entirely independent
of the display pipeline, but doesn't require it really.
-Daniel

> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 172 +++++++++++++++++++++++++++++++++++
>  1 file changed, 172 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 919c901..35f5682 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -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)
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 05/11] drm/tilcdc: Add tilcdc_crtc_atomic_check()

[Jyri Sarha]

Add tilcdc_crtc_atomic_check(). Checks the display mode validity and
the presence of the mandatory primary plane.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 35f5682..2ac9d41 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -481,6 +481,34 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	crtc->hwmode = crtc->state->adjusted_mode;
 }
 
+static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
+				    struct drm_crtc_state *state)
+{
+	struct drm_display_mode *mode = &state->mode;
+	struct drm_display_mode *adjusted_mode = &state->adjusted_mode;
+	int ret;
+
+	if (state->state->plane_states[0]->crtc != crtc ||
+	    state->state->planes[0] != crtc->primary) {
+		dev_dbg(crtc->dev->dev, "CRTC primary plane must be present");
+		return -EINVAL;
+	}
+
+	ret = tilcdc_crtc_mode_valid(crtc, mode);
+	if (ret) {
+		dev_dbg(crtc->dev->dev, "Mode \"%s\" not valid", mode->name);
+		return -EINVAL;
+	}
+
+	if (!tilcdc_crtc_mode_fixup(crtc, mode, adjusted_mode)) {
+		dev_dbg(crtc->dev->dev, "Mode fixup for \"%s\" failed",
+			mode->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
 		struct drm_display_mode *mode,
 		struct drm_display_mode *adjusted_mode,
@@ -697,6 +725,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,
+		.atomic_check	= tilcdc_crtc_atomic_check,
 		.mode_set_nofb	= tilcdc_crtc_mode_set_nofb,
 };
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 06/11] drm/tilcdc: Add atomic mode config funcs

[Jyri Sarha]

Add atomic mode config funcs. The atomic_commit implementation is a
copy-paste from drm_atomic_helper_commit(), leaving out the async
test. The similar copy-paste implementation appears to be used in many
other drivers too. The standard drm_atomic_helper_check() is used for
checking.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 47 +++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 709bc90..66a283e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -20,6 +20,8 @@
 #include <linux/component.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/suspend.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
 
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"
@@ -59,9 +61,54 @@ static void tilcdc_fb_output_poll_changed(struct drm_device *dev)
 	drm_fbdev_cma_hotplug_event(priv->fbdev);
 }
 
+static int tilcdc_commit(struct drm_device *dev,
+		  struct drm_atomic_state *state,
+		  bool async)
+{
+	int ret;
+
+	ret = drm_atomic_helper_prepare_planes(dev, state);
+	if (ret)
+		return ret;
+
+	drm_atomic_helper_swap_state(dev, state);
+
+	/*
+	 * Everything below can be run asynchronously without the need to grab
+	 * any modeset locks at all under one condition: It must be guaranteed
+	 * that the asynchronous work has either been cancelled (if the driver
+	 * supports it, which at least requires that the framebuffers get
+	 * cleaned up with drm_atomic_helper_cleanup_planes()) or completed
+	 * before the new state gets committed on the software side with
+	 * drm_atomic_helper_swap_state().
+	 *
+	 * This scheme allows new atomic state updates to be prepared and
+	 * checked in parallel to the asynchronous completion of the previous
+	 * update. Which is important since compositors need to figure out the
+	 * composition of the next frame right after having submitted the
+	 * current layout.
+	 */
+
+	drm_atomic_helper_commit_modeset_disables(dev, state);
+
+	drm_atomic_helper_commit_planes(dev, state, false);
+
+	drm_atomic_helper_commit_modeset_enables(dev, state);
+
+	drm_atomic_helper_wait_for_vblanks(dev, state);
+
+	drm_atomic_helper_cleanup_planes(dev, state);
+
+	drm_atomic_state_free(state);
+
+	return 0;
+}
+
 static const struct drm_mode_config_funcs mode_config_funcs = {
 	.fb_create = tilcdc_fb_create,
 	.output_poll_changed = tilcdc_fb_output_poll_changed,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = tilcdc_commit,
 };
 
 static int modeset_init(struct drm_device *dev)
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 07/11] drm/tilcdc: Add drm_mode_config_reset() call to tilcdc_load()

[Jyri Sarha]

Add drm_mode_config_reset() call to tilcdc_load(). This is need to
initialize atomic state variables at load time.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 66a283e..dc0d1e9 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -342,6 +342,9 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	drm_helper_disable_unused_functions(dev);
+
+	drm_mode_config_reset(dev);
+
 	priv->fbdev = drm_fbdev_cma_init(dev, bpp,
 			dev->mode_config.num_crtc,
 			dev->mode_config.num_connector);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 08/11] drm/tilcdc: Call drm_crtc_vblank_off() in tilcdc_crtc_destroy()

[Jyri Sarha]

Call drm_crtc_vblank_off() in tilcdc_crtc_destroy(). This is needed
for module unloading after the DRIVER_ATOMIC is enabled.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 2ac9d41..69045d8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -163,7 +163,7 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 
 	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
-
+	drm_crtc_vblank_off(crtc);
 	of_node_put(crtc->port);
 	drm_crtc_cleanup(crtc);
 	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 09/11] drm/tilcdc: Set DRIVER_ATOMIC and use atomic crtc helpers

[Jyri Sarha]

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 <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 +++++++++++-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 69045d8..e8e309e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -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 = {
@@ -725,6 +729,8 @@ 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,
+		.enable		= tilcdc_crtc_enable,
+		.disable	= tilcdc_crtc_disable,
 		.atomic_check	= tilcdc_crtc_atomic_check,
 		.mode_set_nofb	= tilcdc_crtc_mode_set_nofb,
 };
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index dc0d1e9..6569d3a 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -587,7 +587,7 @@ static const struct file_operations fops = {
 
 static struct drm_driver tilcdc_driver = {
 	.driver_features    = (DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET |
-			       DRIVER_PRIME),
+			       DRIVER_PRIME | DRIVER_ATOMIC),
 	.load               = tilcdc_load,
 	.unload             = tilcdc_unload,
 	.lastclose          = tilcdc_lastclose,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 09/11] drm/tilcdc: Set DRIVER_ATOMIC and use atomic crtc helpers

[Jyri Sarha]

On 04/11/16 19:46, Jyri Sarha wrote:
> 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 <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 +++++++++++-----
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  2 +-
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 69045d8..e8e309e 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -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 = {
> @@ -725,6 +729,8 @@ static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
>  		.commit         = tilcdc_crtc_commit,
Oops, I made a compile breakage here in the last phase. The
tilcdc_crtc_commit () and tilcdc_crtc_preapre() above were renamed to
*enable() and *disable(). I'll fix that in the next round.

>  		.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,
>  };
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index dc0d1e9..6569d3a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -587,7 +587,7 @@ static const struct file_operations fops = {
>  
>  static struct drm_driver tilcdc_driver = {
>  	.driver_features    = (DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET |
> -			       DRIVER_PRIME),
> +			       DRIVER_PRIME | DRIVER_ATOMIC),
>  	.load               = tilcdc_load,
>  	.unload             = tilcdc_unload,
>  	.lastclose          = tilcdc_lastclose,
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 10/11] drm/tilcdc: Remove obsolete crtc helper functions

[Jyri Sarha]

Remove obsolete crtc helper functions. These are not needed when
atomic modeset is used.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 209 -----------------------------------
 1 file changed, 209 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index e8e309e..4d019eb8 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -510,209 +510,6 @@ static int tilcdc_crtc_atomic_check(struct drm_crtc *crtc,
 	return 0;
 }
 
-static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
-		struct drm_display_mode *mode,
-		struct drm_display_mode *adjusted_mode,
-		int x, int y,
-		struct drm_framebuffer *old_fb)
-{
-	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;
-	int ret;
-
-	ret = tilcdc_crtc_mode_valid(crtc, mode);
-	if (WARN_ON(ret))
-		return ret;
-
-	if (WARN_ON(!info))
-		return -EINVAL;
-
-	ret = tilcdc_verify_fb(crtc, crtc->primary->fb);
-	if (ret)
-		return ret;
-
-	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:
-		return -EINVAL;
-	}
-	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);
-
-	/* Configure the 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 suppoted.
-	 */
-	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);
-	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(crtc->primary->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 -EINVAL;
-		}
-	}
-	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);
-
-	/*
-	 * use value from adjusted_mode here as this might have been
-	 * changed as part of the fixup for slave encoders to solve the
-	 * issue where tilcdc timings are not VESA compliant
-	 */
-	if (adjusted_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(crtc->primary->fb);
-
-	set_scanout(crtc, crtc->primary->fb);
-
-	tilcdc_crtc_update_clk(crtc);
-
-	pm_runtime_put_sync(dev->dev);
-
-	return 0;
-}
-
-static int tilcdc_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-		struct drm_framebuffer *old_fb)
-{
-	struct drm_device *dev = crtc->dev;
-	int r;
-
-	r = tilcdc_verify_fb(crtc, crtc->primary->fb);
-	if (r)
-		return r;
-
-	drm_framebuffer_reference(crtc->primary->fb);
-
-	pm_runtime_get_sync(dev->dev);
-
-	set_scanout(crtc, crtc->primary->fb);
-
-	pm_runtime_put_sync(dev->dev);
-
-	return 0;
-}
-
 static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
 	.destroy        = tilcdc_crtc_destroy,
 	.set_config     = drm_atomic_helper_set_config,
@@ -723,12 +520,6 @@ static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
 };
 
 static const struct drm_crtc_helper_funcs tilcdc_crtc_helper_funcs = {
-		.dpms           = tilcdc_crtc_dpms,
-		.mode_fixup     = tilcdc_crtc_mode_fixup,
-		.prepare        = tilcdc_crtc_prepare,
-		.commit         = tilcdc_crtc_commit,
-		.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,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC 11/11] drm/tilcdc: Remove tilcdc_verify_fb()

[Jyri Sarha]

Remove tilcdc_verify_fb(). The tilcdc_verify_fb() function is not
needed because the same checks are implemented in
tilcdc_plane_atomic_check().

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 4d019eb8..e572baf 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -170,22 +170,6 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
 }
 
-static int tilcdc_verify_fb(struct drm_crtc *crtc, struct drm_framebuffer *fb)
-{
-	struct drm_device *dev = crtc->dev;
-	unsigned int depth, bpp;
-
-	drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
-
-	if (fb->pitches[0] != crtc->mode.hdisplay * bpp / 8) {
-		dev_err(dev->dev,
-			"Invalid pitch: fb and crtc widths must be the same");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
 		struct drm_framebuffer *fb,
 		struct drm_pending_vblank_event *event,
@@ -193,15 +177,10 @@ int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
-	int r;
 	unsigned long flags;
 	s64 tdiff;
 	ktime_t next_vblank;
 
-	r = tilcdc_verify_fb(crtc, fb);
-	if (r)
-		return r;
-
 	if (tilcdc_crtc->event) {
 		dev_err(dev->dev, "already pending page flip!\n");
 		return -EBUSY;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1605 bytes --]

Hi Jyri,

On 11/04/16 19:46, Jyri Sarha wrote:
> 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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

On Mon, May 09, 2016 at 05:10:00PM +0300, Tomi Valkeinen wrote:
> Hi Jyri,
> 
> On 11/04/16 19:46, Jyri Sarha wrote:
> > 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.

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.

> 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Jyri Sarha]

On 05/09/16 17:42, Daniel Vetter wrote:
>> > 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

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

Well, there is no such thing in LCDC.

>> > 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.


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

	Jyri
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

On Tue, May 10, 2016 at 12:29:23AM +0300, Jyri Sarha wrote:
> On 05/09/16 17:42, Daniel Vetter wrote:
> >> > 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
> 
> > a real plane for the cursor, if you want to support that with atomic.
> > 
> 
> Well, there is no such thing in LCDC.
> 
> >> > 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.
> 
> 
> 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Jyri Sarha]

On 05/10/16 09:34, Daniel Vetter wrote:
>>> 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?
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Noralf Trønnes]

Den 10.05.2016 11:14, skrev Jyri Sarha:
> On 05/10/16 09:34, Daniel Vetter wrote:
>>>> 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?
>> 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?

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

int drm_simple_display_pipe_init(struct drm_device *dev,
                  struct drm_simple_display_pipe *pipe,
                  struct drm_simple_display_pipe_funcs *funcs,
                  const uint32_t *formats,
                  unsigned int format_count,
                  struct drm_encoder *encoder,
                  struct drm_connector *connector)
{
     struct drm_plane *plane = &pipe->plane;
     struct drm_crtc *crtc = &pipe->crtc;
     int ret;

     pipe->funcs = funcs;

     drm_plane_helper_add(plane, &drm_simple_kms_plane_helper_funcs);
     ret = drm_universal_plane_init(dev, plane, 0,
                        &drm_simple_kms_plane_funcs,
                        formats, format_count,
                        DRM_PLANE_TYPE_PRIMARY, NULL);
     if (ret)
         return ret;

     drm_crtc_helper_add(crtc, &drm_simple_kms_crtc_helper_funcs);
     ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
                     &drm_simple_kms_crtc_funcs, NULL);
     if (ret)
         return ret;

     if (!encoder) {
         encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
         if (!encoder)
             return -ENOMEM;

         ret = drm_encoder_init(dev, encoder,
                        &drm_simple_kms_encoder_funcs,
                        DRM_MODE_ENCODER_NONE, NULL);
         if (ret)
             return ret;
     }

     encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
     ret = drm_mode_connector_attach_encoder(connector, encoder);
     if (ret)
         return ret;

     return drm_connector_register(connector);
}

static void drm_simple_kms_encoder_cleanup(struct drm_encoder *encoder)
{
     drm_encoder_cleanup(encoder);
     kfree(encoder);
}

static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
     .destroy = drm_simple_kms_encoder_cleanup,
};


Noralf.

> 	Jyri

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

On Tue, May 10, 2016 at 4:04 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> Den 10.05.2016 11:14, skrev Jyri Sarha:
>>
>> On 05/10/16 09:34, Daniel Vetter wrote:
>>>>>
>>>>> 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?
>>>
>>> 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?
>
>
> 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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Laurent Pinchart]

On Tuesday 10 May 2016 16:18:54 Daniel Vetter wrote:
> On Tue, May 10, 2016 at 4:04 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> > Den 10.05.2016 11:14, skrev Jyri Sarha:
> >> On 05/10/16 09:34, Daniel Vetter wrote:
> >>>>>>> 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?
> >>> 
> >>> 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?
> > 
> > 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).

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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

On Tue, May 10, 2016 at 5:11 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> >> 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:
>>
>> 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.

Hm, just realized that tda998x _is_ an encoder slave driver. But since
it's not automatically wire up like drm_bridge we can't keep the dummy
encoder of drm_simple_display_pipe. Would be good indeed to have a
minimal drm_bridge conversion of tda998x so that we don't need to add
hacks to the simple pipe helpers.

A small function to set the first drm_bridge for a
drm_simple_display_pipe should be all that's really needed here. A bit
more work, but hopefully not much. And really should be worth it to
use the simple helpers for tilcdc, it matches the use-case perfectly I
think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Noralf Trønnes]

Den 10.05.2016 16:18, skrev Daniel Vetter:
> On Tue, May 10, 2016 at 4:04 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>> Den 10.05.2016 11:14, skrev Jyri Sarha:
>>> On 05/10/16 09:34, Daniel Vetter wrote:
>>>>>> 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?
>>>> 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?
>>
>> 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).

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.

Noralf.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

On Tue, May 10, 2016 at 7:30 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
>> 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.

Oops you're right. I think the correct fix would be to always run the
bridge functions though, there's really no need to break that when
there's no encoder vtable. Since I merged your initial patch already,
care to create a follow-up patch to rectify this?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Daniel Vetter]

On Tue, May 10, 2016 at 12:29:23AM +0300, Jyri Sarha wrote:
> On 05/09/16 17:42, Daniel Vetter wrote:
> >> > 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

that's not enough, since if you disable the primary plane it'll still be
part of the state, but disabled. You need to check that it actually is
enabled, and placed correctly (i.e. fullscreen). There's a helper for
that. But if you switch over to drm_simple_display_pipe that helper will
take care of things. At least in the final revision, current patch version
also lacks that check.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 00/11] drm/tilcdc: Atomic modeset support

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 2116 bytes --]

On 05/09/16 17:10, Tomi Valkeinen wrote:
> Hi Jyri,
> 
> On 11/04/16 19:46, Jyri Sarha wrote:
>> 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.
> 

I did not really think of that. I'll make the change.

> 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
> 

I left that for subsequent clean up after general approach is accepted.

> 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.
> 

As modeset_nofb is the preferred call back for setting the mode, and it
does not require any fb or plane, I needed something to express the
mandatory framebuffer. The primatry plane just wast the first solution
that came to my mind and it appeared to work.

> 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
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel