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
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__ */
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; +}
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;
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)
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
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, };
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)
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);
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);
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,
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,}; 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),
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,
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;
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
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
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
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
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
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
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
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.
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
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.
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
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
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jyri Sarha -
Laurent Pinchart -
Noralf Trønnes -
Tomi Valkeinen