Hi,
This patch series introduces a number of useful features for the Tegra DRM driver. The first patch allows VBLANK support to be used in drivers that don't or can't use DRIVER_HAVE_IRQ for interrupt support.
Patch 2 adds support for the additional two planes available on Tegra hardware, while patch 3 implement .mode_set_base() to allow for some nice speed up when changing the framebuffer without actually changing the resolution. Patch 4 adds VBLANK support and finally patch 5 builds on it to provide the page-flipping IOCTL.
Thierry
Thierry Reding (5): drm: Allow vblank support without DRIVER_HAVE_IRQ drm/tegra: Add plane support drm/tegra: Implement .mode_set_base() drm/tegra: Implement VBLANK support drm/tegra: Implement page-flipping support
drivers/gpu/drm/drm_irq.c | 5 +- drivers/gpu/drm/tegra/dc.c | 475 ++++++++++++++++++++++++++++++++++---------- drivers/gpu/drm/tegra/dc.h | 2 +- drivers/gpu/drm/tegra/drm.c | 53 +++++ drivers/gpu/drm/tegra/drm.h | 37 ++++ 5 files changed, 461 insertions(+), 111 deletions(-)
Drivers that register interrupt handlers without the DRM core helpers don't initialize the .irq_enabled field and drm_dev_to_irq() may fail when called on them. This shouldn't preclude them from implementing the vblank IOCTL.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/drm_irq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 19c01ca..71f8205 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1218,8 +1218,9 @@ int drm_wait_vblank(struct drm_device *dev, void *data, int ret; unsigned int flags, seq, crtc, high_crtc;
- if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) - return -EINVAL; + if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ)) + if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) + return -EINVAL;
if (vblwait->request.type & _DRM_VBLANK_SIGNAL) return -EINVAL;
Add support for the B and C planes which support RGB and YUV pixel formats and can be used as overlays or hardware cursor.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 321 +++++++++++++++++++++++++++++++------------- drivers/gpu/drm/tegra/dc.h | 2 +- drivers/gpu/drm/tegra/drm.h | 29 ++++ 3 files changed, 259 insertions(+), 93 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 656b2e3..157e962 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -18,19 +18,104 @@ #include "drm.h" #include "dc.h"
-struct tegra_dc_window { - fixed20_12 x; - fixed20_12 y; - fixed20_12 w; - fixed20_12 h; - unsigned int outx; - unsigned int outy; - unsigned int outw; - unsigned int outh; - unsigned int stride; - unsigned int fmt; +struct tegra_plane { + struct drm_plane base; + unsigned int index; };
+static inline struct tegra_plane *to_tegra_plane(struct drm_plane *plane) +{ + return container_of(plane, struct tegra_plane, base); +} + +static int tegra_plane_update(struct drm_plane *plane, struct drm_crtc *crtc, + struct drm_framebuffer *fb, int crtc_x, + int crtc_y, unsigned int crtc_w, + unsigned int crtc_h, uint32_t src_x, + uint32_t src_y, uint32_t src_w, uint32_t src_h) +{ + unsigned long base = tegra_framebuffer_base(fb); + struct tegra_plane *p = to_tegra_plane(plane); + struct tegra_dc *dc = to_tegra_dc(crtc); + struct tegra_dc_window window; + + memset(&window, 0, sizeof(window)); + window.src.x = src_x >> 16; + window.src.y = src_y >> 16; + window.src.w = src_w >> 16; + window.src.h = src_h >> 16; + window.dst.x = crtc_x; + window.dst.y = crtc_y; + window.dst.w = crtc_w; + window.dst.h = crtc_h; + window.format = tegra_dc_format(fb->pixel_format); + window.bits_per_pixel = fb->bits_per_pixel; + window.stride = fb->pitches[0]; + window.base = base; + + return tegra_dc_setup_window(dc, p->index + 1, &window); +} + +static int tegra_plane_disable(struct drm_plane *plane) +{ + struct tegra_dc *dc = to_tegra_dc(plane->crtc); + struct tegra_plane *p = to_tegra_plane(plane); + unsigned int index = p->index + 1; + unsigned long value; + + value = WINDOW_A_SELECT << index; + tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER); + + value = tegra_dc_readl(dc, DC_WIN_WIN_OPTIONS); + value &= ~WIN_ENABLE; + tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS); + + value = (WIN_A_ACT_REQ << index) | (WIN_A_UPDATE << index); + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); + + return 0; +} + +static void tegra_plane_destroy(struct drm_plane *plane) +{ + drm_plane_cleanup(plane); +} + +static const struct drm_plane_funcs tegra_plane_funcs = { + .update_plane = tegra_plane_update, + .disable_plane = tegra_plane_disable, + .destroy = tegra_plane_destroy, +}; + +static const uint32_t plane_formats[] = { + DRM_FORMAT_XRGB8888, + DRM_FORMAT_YUV422, +}; + +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc) +{ + unsigned int i; + int err = 0; + + for (i = 0; i < 2; i++) { + struct tegra_plane *plane; + + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL); + if (!plane) + return -ENOMEM; + + plane->index = i; + + err = drm_plane_init(drm, &plane->base, 1 << dc->pipe, + &tegra_plane_funcs, plane_formats, + ARRAY_SIZE(plane_formats), false); + if (err < 0) + return err; + } + + return 0; +} + static const struct drm_crtc_funcs tegra_crtc_funcs = { .set_config = drm_crtc_helper_set_config, .destroy = drm_crtc_cleanup, @@ -47,10 +132,11 @@ static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, return true; }
-static inline u32 compute_dda_inc(fixed20_12 inf, unsigned int out, bool v, +static inline u32 compute_dda_inc(unsigned int in, unsigned int out, bool v, unsigned int bpp) { fixed20_12 outf = dfixed_init(out); + fixed20_12 inf = dfixed_init(in); u32 dda_inc; int max;
@@ -80,9 +166,10 @@ static inline u32 compute_dda_inc(fixed20_12 inf, unsigned int out, bool v, return dda_inc; }
-static inline u32 compute_initial_dda(fixed20_12 in) +static inline u32 compute_initial_dda(unsigned int in) { - return dfixed_frac(in); + fixed20_12 inf = dfixed_init(in); + return dfixed_frac(inf); }
static int tegra_dc_set_timings(struct tegra_dc *dc, @@ -153,6 +240,111 @@ static int tegra_crtc_setup_clk(struct drm_crtc *crtc, return 0; }
+int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, + const struct tegra_dc_window *window) +{ + unsigned h_offset, v_offset, h_size, v_size, h_dda, v_dda, bpp; + unsigned long value; + + bpp = window->bits_per_pixel / 8; + + value = WINDOW_A_SELECT << index; + tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER); + + tegra_dc_writel(dc, window->format, DC_WIN_COLOR_DEPTH); + tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP); + + value = V_POSITION(window->dst.y) | H_POSITION(window->dst.x); + tegra_dc_writel(dc, value, DC_WIN_POSITION); + + value = V_SIZE(window->dst.h) | H_SIZE(window->dst.w); + tegra_dc_writel(dc, value, DC_WIN_SIZE); + + h_offset = window->src.x * bpp; + v_offset = window->src.y; + h_size = window->src.w * bpp; + v_size = window->src.h; + + value = V_PRESCALED_SIZE(v_size) | H_PRESCALED_SIZE(h_size); + tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE); + + h_dda = compute_dda_inc(window->src.w, window->dst.w, false, bpp); + v_dda = compute_dda_inc(window->src.h, window->dst.h, true, bpp); + + value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda); + tegra_dc_writel(dc, value, DC_WIN_DDA_INC); + + h_dda = compute_initial_dda(window->src.x); + v_dda = compute_initial_dda(window->src.y); + + tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA); + tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA); + + tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE); + tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE); + + tegra_dc_writel(dc, window->base, DC_WINBUF_START_ADDR); + tegra_dc_writel(dc, window->stride, DC_WIN_LINE_STRIDE); + tegra_dc_writel(dc, h_offset, DC_WINBUF_ADDR_H_OFFSET); + tegra_dc_writel(dc, v_offset, DC_WINBUF_ADDR_V_OFFSET); + + value = WIN_ENABLE; + + if (bpp < 24) + value |= COLOR_EXPAND; + + tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS); + + /* + * Disable blending and assume Window A is the bottom-most window, + * Window C is the top-most window and Window B is in the middle. + */ + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_NOKEY); + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_1WIN); + + switch (index) { + case 0: + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_X); + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_Y); + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_3WIN_XY); + break; + + case 1: + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_X); + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_Y); + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_3WIN_XY); + break; + + case 2: + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_X); + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_Y); + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_3WIN_XY); + break; + } + + value = (WIN_A_ACT_REQ << index) | (WIN_A_UPDATE << index); + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); + + return 0; +} + +unsigned int tegra_dc_format(uint32_t format) +{ + switch (format) { + case DRM_FORMAT_XRGB8888: + return WIN_COLOR_DEPTH_B8G8R8A8; + + case DRM_FORMAT_RGB565: + return WIN_COLOR_DEPTH_B5G6R5; + + default: + break; + } + + WARN(1, "unsupported pixel format %u, using default\n", format); + return WIN_COLOR_DEPTH_B8G8R8A8; +} + static int tegra_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted, @@ -160,8 +352,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, { struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb); struct tegra_dc *dc = to_tegra_dc(crtc); - unsigned int h_dda, v_dda, bpp; - struct tegra_dc_window win; + struct tegra_dc_window window; unsigned long div, value; int err;
@@ -192,81 +383,23 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, tegra_dc_writel(dc, value, DC_DISP_DISP_CLOCK_CONTROL);
/* setup window parameters */ - memset(&win, 0, sizeof(win)); - win.x.full = dfixed_const(0); - win.y.full = dfixed_const(0); - win.w.full = dfixed_const(mode->hdisplay); - win.h.full = dfixed_const(mode->vdisplay); - win.outx = 0; - win.outy = 0; - win.outw = mode->hdisplay; - win.outh = mode->vdisplay; - - switch (crtc->fb->pixel_format) { - case DRM_FORMAT_XRGB8888: - win.fmt = WIN_COLOR_DEPTH_B8G8R8A8; - break; - - case DRM_FORMAT_RGB565: - win.fmt = WIN_COLOR_DEPTH_B5G6R5; - break; - - default: - win.fmt = WIN_COLOR_DEPTH_B8G8R8A8; - WARN_ON(1); - break; - } - - bpp = crtc->fb->bits_per_pixel / 8; - win.stride = crtc->fb->pitches[0]; - - /* program window registers */ - value = WINDOW_A_SELECT; - tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER); - - tegra_dc_writel(dc, win.fmt, DC_WIN_COLOR_DEPTH); - tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP); - - value = V_POSITION(win.outy) | H_POSITION(win.outx); - tegra_dc_writel(dc, value, DC_WIN_POSITION); - - value = V_SIZE(win.outh) | H_SIZE(win.outw); - tegra_dc_writel(dc, value, DC_WIN_SIZE); - - value = V_PRESCALED_SIZE(dfixed_trunc(win.h)) | - H_PRESCALED_SIZE(dfixed_trunc(win.w) * bpp); - tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE); - - h_dda = compute_dda_inc(win.w, win.outw, false, bpp); - v_dda = compute_dda_inc(win.h, win.outh, true, bpp); - - value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda); - tegra_dc_writel(dc, value, DC_WIN_DDA_INC); - - h_dda = compute_initial_dda(win.x); - v_dda = compute_initial_dda(win.y); - - tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA); - tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA); - - tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE); - tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE); - - tegra_dc_writel(dc, fb->obj->paddr, DC_WINBUF_START_ADDR); - tegra_dc_writel(dc, win.stride, DC_WIN_LINE_STRIDE); - tegra_dc_writel(dc, dfixed_trunc(win.x) * bpp, - DC_WINBUF_ADDR_H_OFFSET); - tegra_dc_writel(dc, dfixed_trunc(win.y), DC_WINBUF_ADDR_V_OFFSET); - - value = WIN_ENABLE; - - if (bpp < 24) - value |= COLOR_EXPAND; - - tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS); - - tegra_dc_writel(dc, 0xff00, DC_WIN_BLEND_NOKEY); - tegra_dc_writel(dc, 0xff00, DC_WIN_BLEND_1WIN); + memset(&window, 0, sizeof(window)); + window.src.x = 0; + window.src.y = 0; + window.src.w = mode->hdisplay; + window.src.h = mode->vdisplay; + window.dst.x = 0; + window.dst.y = 0; + window.dst.w = mode->hdisplay; + window.dst.h = mode->vdisplay; + window.format = tegra_dc_format(crtc->fb->pixel_format); + window.bits_per_pixel = crtc->fb->bits_per_pixel; + window.stride = crtc->fb->pitches[0]; + window.base = fb->obj->paddr; + + err = tegra_dc_setup_window(dc, 0, &window); + if (err < 0) + dev_err(dc->dev, "failed to enable root plane\n");
return 0; } @@ -588,7 +721,7 @@ static int tegra_dc_show_regs(struct seq_file *s, void *data) DUMP_REG(DC_WIN_BLEND_1WIN); DUMP_REG(DC_WIN_BLEND_2WIN_X); DUMP_REG(DC_WIN_BLEND_2WIN_Y); - DUMP_REG(DC_WIN_BLEND32WIN_XY); + DUMP_REG(DC_WIN_BLEND_3WIN_XY); DUMP_REG(DC_WIN_HP_FETCH_CONTROL); DUMP_REG(DC_WINBUF_START_ADDR); DUMP_REG(DC_WINBUF_START_ADDR_NS); @@ -690,6 +823,10 @@ static int tegra_dc_drm_init(struct host1x_client *client, return err; }
+ err = tegra_dc_add_planes(drm, dc); + if (err < 0) + return err; + if (IS_ENABLED(CONFIG_DEBUG_FS)) { err = tegra_dc_debugfs_init(dc, drm->primary); if (err < 0) diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h index 99977b5..ccfc220 100644 --- a/drivers/gpu/drm/tegra/dc.h +++ b/drivers/gpu/drm/tegra/dc.h @@ -359,7 +359,7 @@ #define DC_WIN_BLEND_1WIN 0x710 #define DC_WIN_BLEND_2WIN_X 0x711 #define DC_WIN_BLEND_2WIN_Y 0x712 -#define DC_WIN_BLEND32WIN_XY 0x713 +#define DC_WIN_BLEND_3WIN_XY 0x713
#define DC_WIN_HP_FETCH_CONTROL 0x714
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 056a6d9..d7ab6bc 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -31,6 +31,11 @@ static inline struct tegra_framebuffer *to_tegra_fb(struct drm_framebuffer *fb) return container_of(fb, struct tegra_framebuffer, base); }
+static inline unsigned long tegra_framebuffer_base(struct drm_framebuffer *fb) +{ + return to_tegra_fb(fb)->obj->paddr; +} + struct host1x { struct drm_device *drm; struct device *dev; @@ -121,6 +126,30 @@ static inline unsigned long tegra_dc_readl(struct tegra_dc *dc, return readl(dc->regs + (reg << 2)); }
+struct tegra_dc_window { + struct { + unsigned int x; + unsigned int y; + unsigned int w; + unsigned int h; + } src; + struct { + unsigned int x; + unsigned int y; + unsigned int w; + unsigned int h; + } dst; + unsigned int bits_per_pixel; + unsigned int format; + unsigned int stride; + unsigned long base; +}; + +/* from dc.c */ +extern unsigned int tegra_dc_format(uint32_t format); +extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, + const struct tegra_dc_window *window); + struct display { unsigned int width; unsigned int height;
The sequence for replacing the scanout buffer is much shorter than a full mode change operation so implementing this callback considerably speeds up cases where only a new framebuffer is to be scanned out.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 157e962..8dd7d8a 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -116,6 +116,25 @@ static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc) return 0; }
+static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y, + struct tegra_framebuffer *fb) +{ + unsigned long value; + + tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER); + + value = fb->base.offsets[0] + y * fb->base.pitches[0] + + x * fb->base.bits_per_pixel / 8; + + tegra_dc_writel(dc, fb->obj->paddr + value, DC_WINBUF_START_ADDR); + + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ; + value |= GENERAL_UPDATE | WIN_A_UPDATE; + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); + + return 0; +} + static const struct drm_crtc_funcs tegra_crtc_funcs = { .set_config = drm_crtc_helper_set_config, .destroy = drm_crtc_cleanup, @@ -404,6 +423,15 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, return 0; }
+static int tegra_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, + struct drm_framebuffer *old_fb) +{ + struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb); + struct tegra_dc *dc = to_tegra_dc(crtc); + + return tegra_dc_set_base(dc, x, y, fb); +} + static void tegra_crtc_prepare(struct drm_crtc *crtc) { struct tegra_dc *dc = to_tegra_dc(crtc); @@ -483,6 +511,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = { .dpms = tegra_crtc_dpms, .mode_fixup = tegra_crtc_mode_fixup, .mode_set = tegra_crtc_mode_set, + .mode_set_base = tegra_crtc_mode_set_base, .prepare = tegra_crtc_prepare, .commit = tegra_crtc_commit, .load_lut = tegra_crtc_load_lut,
Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller.
While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 55 +++++++++++++++++++++++++++++++-------------- drivers/gpu/drm/tegra/drm.c | 44 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/drm.h | 3 +++ 3 files changed, 85 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 8dd7d8a..3aa7ccc 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -135,6 +135,32 @@ static int tegra_dc_set_base(struct tegra_dc *dc, int x, int y, return 0; }
+void tegra_dc_enable_vblank(struct tegra_dc *dc) +{ + unsigned long value, flags; + + spin_lock_irqsave(&dc->lock, flags); + + value = tegra_dc_readl(dc, DC_CMD_INT_MASK); + value |= VBLANK_INT; + tegra_dc_writel(dc, value, DC_CMD_INT_MASK); + + spin_unlock_irqrestore(&dc->lock, flags); +} + +void tegra_dc_disable_vblank(struct tegra_dc *dc) +{ + unsigned long value, flags; + + spin_lock_irqsave(&dc->lock, flags); + + value = tegra_dc_readl(dc, DC_CMD_INT_MASK); + value &= ~VBLANK_INT; + tegra_dc_writel(dc, value, DC_CMD_INT_MASK); + + spin_unlock_irqrestore(&dc->lock, flags); +} + static const struct drm_crtc_funcs tegra_crtc_funcs = { .set_config = drm_crtc_helper_set_config, .destroy = drm_crtc_cleanup, @@ -375,6 +401,8 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, unsigned long div, value; int err;
+ drm_vblank_pre_modeset(crtc->dev, dc->pipe); + err = tegra_crtc_setup_clk(crtc, mode, &div); if (err) { dev_err(dc->dev, "failed to setup clock for CRTC: %d\n", err); @@ -476,31 +504,23 @@ static void tegra_crtc_prepare(struct drm_crtc *crtc) tegra_dc_writel(dc, value, DC_DISP_DISP_MEM_HIGH_PRIORITY_TIMER);
value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT; - tegra_dc_writel(dc, value, DC_CMD_INT_MASK); - - value = VBLANK_INT | WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT; tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE); + + value = WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT; + tegra_dc_writel(dc, value, DC_CMD_INT_MASK); }
static void tegra_crtc_commit(struct drm_crtc *crtc) { struct tegra_dc *dc = to_tegra_dc(crtc); - unsigned long update_mask; unsigned long value;
- update_mask = GENERAL_ACT_REQ | WIN_A_ACT_REQ; - - tegra_dc_writel(dc, update_mask << 8, DC_CMD_STATE_CONTROL); - - value = tegra_dc_readl(dc, DC_CMD_INT_ENABLE); - value |= FRAME_END_INT; - tegra_dc_writel(dc, value, DC_CMD_INT_ENABLE); + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ | + GENERAL_UPDATE | WIN_A_UPDATE;
- value = tegra_dc_readl(dc, DC_CMD_INT_MASK); - value |= FRAME_END_INT; - tegra_dc_writel(dc, value, DC_CMD_INT_MASK); + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
- tegra_dc_writel(dc, update_mask, DC_CMD_STATE_CONTROL); + drm_vblank_post_modeset(crtc->dev, dc->pipe); }
static void tegra_crtc_load_lut(struct drm_crtc *crtc) @@ -517,7 +537,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = { .load_lut = tegra_crtc_load_lut, };
-static irqreturn_t tegra_drm_irq(int irq, void *data) +static irqreturn_t tegra_dc_irq(int irq, void *data) { struct tegra_dc *dc = data; unsigned long status; @@ -862,7 +882,7 @@ static int tegra_dc_drm_init(struct host1x_client *client, dev_err(dc->dev, "debugfs setup failed: %d\n", err); }
- err = devm_request_irq(dc->dev, dc->irq, tegra_drm_irq, 0, + err = devm_request_irq(dc->dev, dc->irq, tegra_dc_irq, 0, dev_name(dc->dev), dc); if (err < 0) { dev_err(dc->dev, "failed to request IRQ#%u: %d\n", dc->irq, @@ -911,6 +931,7 @@ static int tegra_dc_probe(struct platform_device *pdev) if (!dc) return -ENOMEM;
+ spin_lock_init(&dc->lock); INIT_LIST_HEAD(&dc->list); dc->dev = &pdev->dev;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 3a503c9..62f8121 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -40,6 +40,10 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags) if (err < 0) return err;
+ err = drm_vblank_init(drm, drm->mode_config.num_crtc); + if (err < 0) + return err; + err = tegra_drm_fb_init(drm); if (err < 0) return err; @@ -89,6 +93,42 @@ static const struct file_operations tegra_drm_fops = { .llseek = noop_llseek, };
+static struct drm_crtc *tegra_crtc_from_pipe(struct drm_device *drm, int pipe) +{ + struct drm_crtc *crtc; + + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) { + struct tegra_dc *dc = to_tegra_dc(crtc); + + if (dc->pipe == pipe) + return crtc; + } + + return NULL; +} + +static int tegra_drm_enable_vblank(struct drm_device *drm, int pipe) +{ + struct drm_crtc *crtc = tegra_crtc_from_pipe(drm, pipe); + struct tegra_dc *dc = to_tegra_dc(crtc); + + if (!crtc) + return -ENODEV; + + tegra_dc_enable_vblank(dc); + + return 0; +} + +static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe) +{ + struct drm_crtc *crtc = tegra_crtc_from_pipe(drm, pipe); + struct tegra_dc *dc = to_tegra_dc(crtc); + + if (crtc) + tegra_dc_disable_vblank(dc); +} + struct drm_driver tegra_drm_driver = { .driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM, .load = tegra_drm_load, @@ -96,6 +136,10 @@ struct drm_driver tegra_drm_driver = { .open = tegra_drm_open, .lastclose = tegra_drm_lastclose,
+ .get_vblank_counter = drm_vblank_count, + .enable_vblank = tegra_drm_enable_vblank, + .disable_vblank = tegra_drm_disable_vblank, + .gem_free_object = drm_gem_cma_free_object, .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create, diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index d7ab6bc..bb5e9d3 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -83,6 +83,7 @@ struct tegra_output;
struct tegra_dc { struct host1x_client client; + spinlock_t lock;
struct host1x *host1x; struct device *dev; @@ -149,6 +150,8 @@ struct tegra_dc_window { extern unsigned int tegra_dc_format(uint32_t format); extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, const struct tegra_dc_window *window); +extern void tegra_dc_enable_vblank(struct tegra_dc *dc); +extern void tegra_dc_disable_vblank(struct tegra_dc *dc);
struct display { unsigned int width;
On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller.
While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de
Sorry Thierry, but is this a useful feature? Are applications really making use of it? Because it means that that an IRQ will have to trigger every 16.67ms when it is taken, which means the device can't sleep. (probably OK because it should go back to idle when the app lets go of the vblank). But worse, for one-shot panels there is no continuous vblank. I've been doing weird hacks to run a timer when the smart panel is idle to trick applications into thinking they have a vblank pulse. But really I think the whole feature of a vblank pulse is pretty lame and I wish it would die. Were you going to use it for something specific, or just adding it for completeness?
- Jon
On 01/23/2013 02:21 AM, Jon Mayo wrote:
On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller.
While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de
Sorry Thierry, but is this a useful feature? Are applications really making use of it? Because it means that that an IRQ will have to trigger every 16.67ms when it is taken, which means the device can't sleep. (probably OK because it should go back to idle when the app lets go of the vblank). But worse, for one-shot panels there is no continuous vblank. I've been doing weird hacks to run a timer when the smart panel is idle to trick applications into thinking they have a vblank pulse. But really I think the whole feature of a vblank pulse is pretty lame and I wish it would die. Were you going to use it for something specific, or just adding it for completeness?
I guess people don't use this to really wait vblanks because that can be done by polling drm fd. Normally they use this ioctl to get the vblank counts which may be useful for their applications.
Mark
- Jon
-- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 22, 2013 at 10:21:40AM -0800, Jon Mayo wrote:
On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller.
While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de
Sorry Thierry, but is this a useful feature? Are applications really making use of it? Because it means that that an IRQ will have to trigger every 16.67ms when it is taken, which means the device can't sleep. (probably OK because it should go back to idle when the app lets go of the vblank). But worse, for one-shot panels there is no continuous vblank. I've been doing weird hacks to run a timer when the smart panel is idle to trick applications into thinking they have a vblank pulse. But really I think the whole feature of a vblank pulse is pretty lame and I wish it would die. Were you going to use it for something specific, or just adding it for completeness?
This is mainly added for completeness. I know that on Tegra we can do a lot better by using syncpoints, but applications such as Weston (in general applications that use the generic DRM API) rely on this to sync rendering with VBLANK.
I don't think there's another way to achieve the same thing. And as you already mentioned, this is only enabled when an application actively uses it, in which case the device won't sleep anyway.
Thierry
On Wed, Jan 30, 2013 at 9:34 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
On Tue, Jan 22, 2013 at 10:21:40AM -0800, Jon Mayo wrote:
On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller.
While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de
Sorry Thierry, but is this a useful feature? Are applications really making use of it? Because it means that that an IRQ will have to trigger every 16.67ms when it is taken, which means the device can't sleep. (probably OK because it should go back to idle when the app lets go of the vblank). But worse, for one-shot panels there is no continuous vblank. I've been doing weird hacks to run a timer when the smart panel is idle to trick applications into thinking they have a vblank pulse. But really I think the whole feature of a vblank pulse is pretty lame and I wish it would die. Were you going to use it for something specific, or just adding it for completeness?
This is mainly added for completeness. I know that on Tegra we can do a lot better by using syncpoints, but applications such as Weston (in general applications that use the generic DRM API) rely on this to sync rendering with VBLANK.
I don't think there's another way to achieve the same thing. And as you already mentioned, this is only enabled when an application actively uses it, in which case the device won't sleep anyway.
I think driving animations with the vblank signal is nice, but we kinda only need that with the pageflip support. Unfortunately the current drm code is a bit a mess in that area, so pageflips force you to enable the vblank interrupt for otherwise the timestamp'ed pageflip completion events won't work.
Recent intel hw has pageflip timestamp registers, so we don't really need that. And I guess other hw is similar. So we probably should clean up and untangle the infrastructure a bit around the drm vblank support code.
Another issue is that the vblank ioctl itself doesn't deal with modeset crtc ids, so adding a new interface for that would be good. Otoh most kms clients don't really use it, but only care about pageflips. -Daniel
On 30.01.13 13:36, Daniel Vetter wrote:
On Wed, Jan 30, 2013 at 9:34 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
On Tue, Jan 22, 2013 at 10:21:40AM -0800, Jon Mayo wrote:
On Mon, Jan 14, 2013 at 7:55 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
Implement support for the VBLANK IOCTL. Note that Tegra is somewhat special in this case because it doesn't use the generic IRQ support provided by the DRM core (DRIVER_HAVE_IRQ) but rather registers one interrupt handler for each display controller.
While at it, clean up the way that interrupts are enabled to ensure that the VBLANK interrupt only gets enabled when required.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de
Sorry Thierry, but is this a useful feature? Are applications really making use of it? Because it means that that an IRQ will have to trigger every 16.67ms when it is taken, which means the device can't sleep. (probably OK because it should go back to idle when the app lets go of the vblank). But worse, for one-shot panels there is no continuous vblank. I've been doing weird hacks to run a timer when the smart panel is idle to trick applications into thinking they have a vblank pulse. But really I think the whole feature of a vblank pulse is pretty lame and I wish it would die. Were you going to use it for something specific, or just adding it for completeness?
This is mainly added for completeness. I know that on Tegra we can do a lot better by using syncpoints, but applications such as Weston (in general applications that use the generic DRM API) rely on this to sync rendering with VBLANK.
I don't think there's another way to achieve the same thing. And as you already mentioned, this is only enabled when an application actively uses it, in which case the device won't sleep anyway.
I think driving animations with the vblank signal is nice, but we kinda only need that with the pageflip support. Unfortunately the current drm code is a bit a mess in that area, so pageflips force you to enable the vblank interrupt for otherwise the timestamp'ed pageflip completion events won't work.
Extensions like SGI_video_sync, OML_sync_control, INTEL_swap_events expose vblank counts and related functionality for synchronizing to the vblank and some applications really use and need them in addition to the pageflip timestamps, so you need a running reliable vblank counter to support these. Some scienctific/medical applications need to also synchronize things like audio playback or capture, or triggering of special research equipment (fMRI/MEG/EEG brain scanners and recordign equipment, eye trackers, TMS brain stimulators etc.) to the vblank of a completed or future bufferswap, with sometimes better than 1 msec precision, sometimes ahead of the event.
That we can't timestamp non-pageflipped swaps precisely/reliably is a limitation, not a feature for such uses.
Recent intel hw has pageflip timestamp registers, so we don't really need that. And I guess other hw is similar. So we probably should clean up and untangle the infrastructure a bit around the drm vblank support code.
At least AMD hw as of a while ago didn't, older intel hw didn't (afaik), and NVidia i don't know. You'd also need hw vblank timestamps and counters independent of page-flip completion to not regress existing important functionality if you want to get rid of the irq based method.
-mario
Another issue is that the vblank ioctl itself doesn't deal with modeset crtc ids, so adding a new interface for that would be good. Otoh most kms clients don't really use it, but only care about pageflips. -Daniel
On Thu, Jan 31, 2013 at 3:06 AM, Mario Kleiner mario.kleiner@tuebingen.mpg.de wrote:
At least AMD hw as of a while ago didn't, older intel hw didn't (afaik), and NVidia i don't know. You'd also need hw vblank timestamps and counters independent of page-flip completion to not regress existing important functionality if you want to get rid of the irq based method.
Intel hw has counters+timestamps for pageflips and vblanks, so you get the cake and eat it, too. I'm also aware of your stringent requirements wrt timestamps, so the recently pimped kms_flip test we have for drm/i915.ko checks vblank timestamps, pageflip timestamps and whether those don't get out of sync somehow in a rather anal way. Results thus far is that the current vblank stuff is (at least for intel) pretty horribly broken - QA reported random and sometimes not-so-random failures across all generations.
So we still have plenty of fixing to do until we can get around to implement pageflip timestamping without the irq vblank running all the time. -Daniel
All the necessary support bits like .mode_set_base() and VBLANK are now available, so page-flipping case easily be implemented on top.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/drm.c | 9 ++++++ drivers/gpu/drm/tegra/drm.h | 5 ++++ 3 files changed, 86 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 3aa7ccc..ce4e14a 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -161,7 +161,78 @@ void tegra_dc_disable_vblank(struct tegra_dc *dc) spin_unlock_irqrestore(&dc->lock, flags); }
+static void tegra_dc_finish_page_flip(struct tegra_dc *dc) +{ + struct drm_pending_vblank_event *event; + struct drm_device *drm = dc->base.dev; + unsigned long flags; + struct timeval now; + + spin_lock_irqsave(&drm->event_lock, flags); + event = dc->event; + dc->event = NULL; + spin_unlock_irqrestore(&drm->event_lock, flags); + + if (!event) + return; + + event->event.sequence = drm_vblank_count_and_time(drm, dc->pipe, &now); + event->event.tv_sec = now.tv_sec; + event->event.tv_usec = now.tv_usec; + + spin_lock_irqsave(&drm->event_lock, flags); + list_add_tail(&event->base.link, &event->base.file_priv->event_list); + wake_up_interruptible(&event->base.file_priv->event_wait); + spin_unlock_irqrestore(&drm->event_lock, flags); + + drm_vblank_put(drm, dc->pipe); +} + +void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file) +{ + struct tegra_dc *dc = to_tegra_dc(crtc); + struct drm_device *drm = crtc->dev; + unsigned long flags; + + spin_lock_irqsave(&drm->event_lock, flags); + + if (dc->event && dc->event->base.file_priv == file) { + dc->event->base.destroy(&dc->event->base); + drm_vblank_put(drm, dc->pipe); + dc->event = NULL; + } + + spin_unlock_irqrestore(&drm->event_lock, flags); +} + +static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, + struct drm_pending_vblank_event *event) +{ + struct tegra_framebuffer *newfb = to_tegra_fb(fb); + struct tegra_dc *dc = to_tegra_dc(crtc); + struct drm_device *drm = crtc->dev; + unsigned long flags; + + if (dc->event) + return -EBUSY; + + tegra_dc_set_base(dc, 0, 0, newfb); + + if (event) { + event->pipe = dc->pipe; + + spin_lock_irqsave(&drm->event_lock, flags); + dc->event = event; + spin_unlock_irqrestore(&drm->event_lock, flags); + + drm_vblank_get(drm, dc->pipe); + } + + return 0; +} + static const struct drm_crtc_funcs tegra_crtc_funcs = { + .page_flip = tegra_dc_page_flip, .set_config = drm_crtc_helper_set_config, .destroy = drm_crtc_cleanup, }; @@ -556,6 +627,7 @@ static irqreturn_t tegra_dc_irq(int irq, void *data) dev_dbg(dc->dev, "%s(): vertical blank\n", __func__); */ drm_handle_vblank(dc->base.dev, dc->pipe); + tegra_dc_finish_page_flip(dc); }
if (status & (WIN_A_UF_INT | WIN_B_UF_INT | WIN_C_UF_INT)) { diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 62f8121..7bab784 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -129,11 +129,20 @@ static void tegra_drm_disable_vblank(struct drm_device *drm, int pipe) tegra_dc_disable_vblank(dc); }
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{ + struct drm_crtc *crtc; + + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head) + tegra_dc_cancel_page_flip(crtc, file); +} + struct drm_driver tegra_drm_driver = { .driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM, .load = tegra_drm_load, .unload = tegra_drm_unload, .open = tegra_drm_open, + .preclose = tegra_drm_preclose, .lastclose = tegra_drm_lastclose,
.get_vblank_counter = drm_vblank_count, diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index bb5e9d3..3aa21b1 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -103,6 +103,9 @@ struct tegra_dc { struct drm_info_list *debugfs_files; struct drm_minor *minor; struct dentry *debugfs; + + /* page-flip handling */ + struct drm_pending_vblank_event *event; };
static inline struct tegra_dc *host1x_client_to_dc(struct host1x_client *client) @@ -152,6 +155,8 @@ extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, const struct tegra_dc_window *window); extern void tegra_dc_enable_vblank(struct tegra_dc *dc); extern void tegra_dc_disable_vblank(struct tegra_dc *dc); +extern void tegra_dc_cancel_page_flip(struct drm_crtc *crtc, + struct drm_file *file);
struct display { unsigned int width;
On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{
struct drm_crtc *crtc;list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)tegra_dc_cancel_page_flip(crtc, file);+}
Why that? If userspace dies while a flip is outstanding, it's imo ok to execute it - otherwise there might be an accounting mismatch if the hw still scans out the old fb, but drm believes the new one is used. Or do I miss something?
The reason I've skimmed through the patches is to check for implications with my modeset locking rework. Review on that would be highly appreciated ...
Thanks, Daniel
On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{
struct drm_crtc *crtc;list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)tegra_dc_cancel_page_flip(crtc, file);+}
Why that? If userspace dies while a flip is outstanding, it's imo ok to execute it - otherwise there might be an accounting mismatch if the hw still scans out the old fb, but drm believes the new one is used. Or do I miss something?
I looked at the shmobile driver for inspiration and they do this as well. Doing so seemed reasonable since there'd be no file to deliver the event to.
The reason I've skimmed through the patches is to check for implications with my modeset locking rework. Review on that would be highly appreciated ...
I'm not sure how suited I am for review given my limited experience, but I'll see if I can make some time to take a look.
Thierry
On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{
struct drm_crtc *crtc;list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)tegra_dc_cancel_page_flip(crtc, file);+}
Why that? If userspace dies while a flip is outstanding, it's imo ok to execute it - otherwise there might be an accounting mismatch if the hw still scans out the old fb, but drm believes the new one is used. Or do I miss something?
I looked at the shmobile driver for inspiration and they do this as well. Doing so seemed reasonable since there'd be no file to deliver the event to.
Hm, is the code in drm_events_release not good enough? And if it's buggy, we need to fix it. Also adding Laurent to figure out why he added that code in shmob ...
The reason I've skimmed through the patches is to check for implications with my modeset locking rework. Review on that would be highly appreciated ...
I'm not sure how suited I am for review given my limited experience, but I'll see if I can make some time to take a look.
The commit message should nicely explain why I've picked the design and the various implications for drivers. So just checking whether anything collides with your upcoming stuff would be good ... -Daniel
On Wed, Jan 16, 2013 at 10:43:15AM +0100, Daniel Vetter wrote:
On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{
struct drm_crtc *crtc;list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)tegra_dc_cancel_page_flip(crtc, file);+}
Why that? If userspace dies while a flip is outstanding, it's imo ok to execute it - otherwise there might be an accounting mismatch if the hw still scans out the old fb, but drm believes the new one is used. Or do I miss something?
I looked at the shmobile driver for inspiration and they do this as well. Doing so seemed reasonable since there'd be no file to deliver the event to.
Hm, is the code in drm_events_release not good enough? And if it's buggy, we need to fix it. Also adding Laurent to figure out why he added that code in shmob ...
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
The reason I've skimmed through the patches is to check for implications with my modeset locking rework. Review on that would be highly appreciated ...
I'm not sure how suited I am for review given my limited experience, but I'll see if I can make some time to take a look.
The commit message should nicely explain why I've picked the design and the various implications for drivers. So just checking whether anything collides with your upcoming stuff would be good ...
Okay, I'll take a closer look.
Thierry
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
I think we need a slightly more generally useful solution, since most drivers are currently broken. I've read a bit through the code, but short of refcounting drm events and adding event->file_priv checks at relevent places I don't see a sane solution ... And even that one is rather invasive. Do you have an idea? Imo doing the cleanup in each driver will be rather error-prone, and since usually kms clients wait for flips to complete, also guaranteed to be little tested. -Daniel
On Wed, Jan 16, 2013 at 6:36 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
I think we need a slightly more generally useful solution, since most drivers are currently broken. I've read a bit through the code, but short of refcounting drm events and adding event->file_priv checks at relevent places I don't see a sane solution ... And even that one is rather invasive. Do you have an idea? Imo doing the cleanup in each driver will be rather error-prone, and since usually kms clients wait for flips to complete, also guaranteed to be little tested.
I suppose we could move the *pending_vblank_event to 'struct drm_crtc'.. I think probably all/most drivers need the same thing anyway. If a driver needs to do something special, it could just never set crtc->pending_vblank_event and instead do it's own cleanup.
BR, -R
-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 http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
I think we need a slightly more generally useful solution, since most drivers are currently broken. I've read a bit through the code, but short of refcounting drm events and adding event->file_priv checks at relevent places I don't see a sane solution ... And even that one is rather invasive. Do you have an idea? Imo doing the cleanup in each driver will be rather error-prone, and since usually kms clients wait for flips to complete, also guaranteed to be little tested.
While this probably doesn't improve the situation much, maybe adding more extensive tests to libdrm or so would help. I wrote a couple of small programs to test vblank and plane support.
The vblank one basically generates two framebuffers with different patterns and uses page-flipping to alternate between them. The plane test does something similar, sets up a plane and associates a buffer with it. It includes scaling the plane to test that functionality as well.
Perhaps these tests could even be added to the existing libdrm tests, but maybe having separate binaries wouldn't hurt.
Back to the original topic: should it not work to queue a page flip and immediately exit(0) after that to test the cleanup code? I suppose if that can be tested on all drivers we should have pretty good coverage.
Thierry
Hi Thierry,
On Wednesday 16 January 2013 19:56:06 Thierry Reding wrote:
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
I think we need a slightly more generally useful solution, since most drivers are currently broken. I've read a bit through the code, but short of refcounting drm events and adding event->file_priv checks at relevent places I don't see a sane solution ... And even that one is rather invasive. Do you have an idea? Imo doing the cleanup in each driver will be rather error-prone, and since usually kms clients wait for flips to complete, also guaranteed to be little tested.
While this probably doesn't improve the situation much, maybe adding more extensive tests to libdrm or so would help. I wrote a couple of small programs to test vblank and plane support.
The vblank one basically generates two framebuffers with different patterns and uses page-flipping to alternate between them. The plane test does something similar, sets up a plane and associates a buffer with it. It includes scaling the plane to test that functionality as well.
Perhaps these tests could even be added to the existing libdrm tests, but maybe having separate binaries wouldn't hurt.
Further cleanup of the modetest application is somewhere on my to-do list (but probably so low that I'll never get to it unless there's a real incentive ;-)). It's a good candidate for more page flip testing (there's basic page flip support there already).
Back to the original topic: should it not work to queue a page flip and immediately exit(0) after that to test the cleanup code? I suppose if that can be tested on all drivers we should have pretty good coverage.
Maybe we need some kind of compliance test suite tool ?
On Sat, Feb 2, 2013 at 12:05 AM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Back to the original topic: should it not work to queue a page flip and immediately exit(0) after that to test the cleanup code? I suppose if that can be tested on all drivers we should have pretty good coverage.
Maybe we need some kind of compliance test suite tool ?
kms_flip from our intel-specific testsuite is probably the most paranoid thing for testing page flips out there:
http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/kms_flip.c
Cheers, Daniel
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
I think I may just recently have run into this bug on Intel hardware. Although perhaps I just used this wrongly.
Just for the fun of it I wanted to implement Conway's Game of Life on top of DRM/KMS. So I use two dumb buffer objects to alternately render to. Then I wanted to use page-flipping to synchronize with VBLANK.
So the sequence is basically:
while (!done) { grid_tick(grid); grid_draw(grid, screen); screen_flip(screen); grid_swap(grid); }
Where screen_flip() chooses the framebuffer and passes it to drmModePageFlip() like so:
int fb = screen->fb[screen->current];
drmModePageFlip(screen->fd, screen->crtc, fb, DRM_MODE_PAGE_FLIP_EVENT, screen);
This runs for about 3 seconds and then hangs, so the display is no longer updated. I've also verified that the same happens on Radeon. But maybe I am mistaken and this isn't the proper programming sequence?
Thierry
On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote:
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
I think I may just recently have run into this bug on Intel hardware. Although perhaps I just used this wrongly.
Just for the fun of it I wanted to implement Conway's Game of Life on top of DRM/KMS. So I use two dumb buffer objects to alternately render to. Then I wanted to use page-flipping to synchronize with VBLANK.
So the sequence is basically:
while (!done) { grid_tick(grid); grid_draw(grid, screen); screen_flip(screen); grid_swap(grid); }
Where screen_flip() chooses the framebuffer and passes it to drmModePageFlip() like so:
int fb = screen->fb[screen->current];
drmModePageFlip(screen->fd, screen->crtc, fb, DRM_MODE_PAGE_FLIP_EVENT, screen);
This runs for about 3 seconds and then hangs, so the display is no longer updated. I've also verified that the same happens on Radeon. But maybe I am mistaken and this isn't the proper programming sequence?
You asked for page flip events. Do you actually handle them in your code?
On Wed, Jan 30, 2013 at 11:42:40AM +0200, Ville Syrjälä wrote:
On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote:
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
I think I may just recently have run into this bug on Intel hardware. Although perhaps I just used this wrongly.
Just for the fun of it I wanted to implement Conway's Game of Life on top of DRM/KMS. So I use two dumb buffer objects to alternately render to. Then I wanted to use page-flipping to synchronize with VBLANK.
So the sequence is basically:
while (!done) { grid_tick(grid); grid_draw(grid, screen); screen_flip(screen); grid_swap(grid); }
Where screen_flip() chooses the framebuffer and passes it to drmModePageFlip() like so:
int fb = screen->fb[screen->current];
drmModePageFlip(screen->fd, screen->crtc, fb, DRM_MODE_PAGE_FLIP_EVENT, screen);
This runs for about 3 seconds and then hangs, so the display is no longer updated. I've also verified that the same happens on Radeon. But maybe I am mistaken and this isn't the proper programming sequence?
You asked for page flip events. Do you actually handle them in your code?
Duh. No I wasn't =) I suppose some queue must be running full if the event isn't handled by calling drmHandleEvent(). Okay, this now works properly with page-flipping.
Thanks. Thierry
On Wed, Jan 30, 2013 at 12:14:36PM +0100, Thierry Reding wrote:
On Wed, Jan 30, 2013 at 11:42:40AM +0200, Ville Syrjälä wrote:
On Wed, Jan 30, 2013 at 10:32:47AM +0100, Thierry Reding wrote:
On Wed, Jan 16, 2013 at 01:36:17PM +0100, Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding thierry.reding@avionic-design.de wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
I think I may just recently have run into this bug on Intel hardware. Although perhaps I just used this wrongly.
Just for the fun of it I wanted to implement Conway's Game of Life on top of DRM/KMS. So I use two dumb buffer objects to alternately render to. Then I wanted to use page-flipping to synchronize with VBLANK.
So the sequence is basically:
while (!done) { grid_tick(grid); grid_draw(grid, screen); screen_flip(screen); grid_swap(grid); }
Where screen_flip() chooses the framebuffer and passes it to drmModePageFlip() like so:
int fb = screen->fb[screen->current];
drmModePageFlip(screen->fd, screen->crtc, fb, DRM_MODE_PAGE_FLIP_EVENT, screen);
This runs for about 3 seconds and then hangs, so the display is no longer updated. I've also verified that the same happens on Radeon. But maybe I am mistaken and this isn't the proper programming sequence?
You asked for page flip events. Do you actually handle them in your code?
Duh. No I wasn't =) I suppose some queue must be running full if the event isn't handled by calling drmHandleEvent(). Okay, this now works properly with page-flipping.
Just in case anybody's interested, the code is here:
https://gitorious.org/thierryreding/kmslife
Thierry
On Wednesday 16 January 2013 13:36:17 Daniel Vetter wrote:
On Wed, Jan 16, 2013 at 11:01 AM, Thierry Reding wrote:
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
Hm, indeed we seem to have a nice bug in most drivers there :(
That's not the only reason.
drm_events_release() handles vblank events added to the vblank_event_list. That list only contains vblank events added by drm_queue_vblank_event(), only called by drm_wait_vblank(). Page flip events never get there, so they need to be cleaned up manually.
I wrote in the DRM documentation, in the page flipping section,
"FIXME: Could drivers that don't need to wait for rendering to complete just add the event to dev->vblank_event_list and let the DRM core handle everything, as for "normal" vertical blanking events?"
We should investigate that.
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
I think we need a slightly more generally useful solution, since most drivers are currently broken. I've read a bit through the code, but short of refcounting drm events and adding event->file_priv checks at relevent places I don't see a sane solution ... And even that one is rather invasive. Do you have an idea? Imo doing the cleanup in each driver will be rather error-prone, and since usually kms clients wait for flips to complete, also guaranteed to be little tested.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jon Mayo -
Laurent Pinchart -
Mario Kleiner -
Mark Zhang -
Rob Clark -
Thierry Reding -
Ville Syrjälä