This patch series introduces a number of useful features for the Tegra DRM driver. Patch 1 is a documentation update and adds a warning to the page-flip IOCTL to catch buggy drivers that return success but failed to update the crtc->fb field, which would cause the reference counting to become unbalanced.
Patch 2 removes usage of the tegra_framebuffer structure which was used before the driver moved to the CMA FB helpers. It is wrong to use it but it happens to work because the drm_fb_cma and tegra_framebuffers were sufficiently similar.
Patch 3 adds support for the additional two planes available on Tegra hardware, while patch 4 implement .mode_set_base() to allow for some nice speed up when changing the framebuffer without actually changing the resolution. Patch 5 adds VBLANK support and patch 6 builds on the previous two to provide the page-flipping IOCTL. Patch 7 splits the DC_CMD_STATE_CONTROL register writes into two consecutive writes as required by the TRM.
Finally patch 8 fixes color expansions for pixel formats with less than 24 bits per pixel and patch 9 adds a new file, named "framebuffers" to debugfs which can be used to dump a list of framebuffers attached to the DRM device. This is most useful to inspect the reference count but could also be helpful in diagnosing out-of-memory conditions and such.
This series is based on Dave's drm-next branch.
Thierry
Thierry Reding (9): drm: Add consistency check for page-flipping drm/tegra: Remove bogus tegra_framebuffer structure drm/tegra: Add plane support drm/tegra: Implement .mode_set_base() drm/tegra: Implement VBLANK support drm/tegra: Implement page-flipping support drm/tegra: Split DC_CMD_STATE_CONTROL register write drm/tegra: Fix color expansion drm/tegra: Add list of framebuffers to debugfs
Documentation/DocBook/drm.tmpl | 6 + drivers/gpu/drm/drm_crtc.c | 7 + drivers/gpu/drm/tegra/dc.c | 585 +++++++++++++++++++++++++++++++++-------- drivers/gpu/drm/tegra/dc.h | 14 +- drivers/gpu/drm/tegra/drm.c | 103 ++++++++ drivers/gpu/drm/tegra/drm.h | 43 ++- 6 files changed, 635 insertions(+), 123 deletions(-)
Driver implementations of the drm_crtc's .page_flip() function are required to update the crtc->fb field on success to reflect that the new framebuffer is now in use. This is important to keep reference counting on the framebuffers balanced.
While at it, document this requirement to keep others from falling into the same trap.
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding thierry.reding@avionic-design.de Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- Documentation/DocBook/drm.tmpl | 6 ++++++ drivers/gpu/drm/drm_crtc.c | 7 +++++++ 2 files changed, 13 insertions(+)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 51e1904..a6428dd 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -1161,6 +1161,12 @@ int max_width, max_height;</synopsis> any new rendering to the frame buffer until the page flip completes. </para> <para> + If a page flip can be successfully scheduled the driver must set the + <code>drm_crtc-<fb</code> field to the new framebuffer pointed to + by <code>fb</code>. This is important so that the reference counting + on framebuffers stays balanced. + </para> + <para> If a page flip is already pending, the <methodname>page_flip</methodname> operation must return -<errorname>EBUSY</errorname>. diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 781aef5..3bdf2a6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3792,6 +3792,13 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, /* Keep the old fb, don't unref it. */ old_fb = NULL; } else { + /* + * Warn if the driver hasn't properly updated the crtc->fb + * field to reflect that the new framebuffer is now used. + * Failing to do so will screw with the reference counting + * on framebuffers. + */ + WARN_ON(crtc->fb != fb); /* Unref only the old framebuffer. */ fb = NULL; }
Tegra uses the CMA FB helpers so framebuffers passed to the driver need to use the corresponding functions to access the underlying GEM objects.
This used to work because struct tegra_framebuffer was sufficiently similar to struct drm_fb_cma but that isn't guaranteed to stay that way.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 2 +- drivers/gpu/drm/tegra/drm.h | 11 ----------- 2 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 656b2e3..d35ff8b 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -158,7 +158,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *adjusted, int x, int y, struct drm_framebuffer *old_fb) { - struct tegra_framebuffer *fb = to_tegra_fb(crtc->fb); + struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(crtc->fb, 0); struct tegra_dc *dc = to_tegra_dc(crtc); unsigned int h_dda, v_dda, bpp; struct tegra_dc_window win; diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 741b5dc..3c61aab 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -18,16 +18,6 @@ #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fixed.h>
-struct tegra_framebuffer { - struct drm_framebuffer base; - struct drm_gem_cma_object *obj; -}; - -static inline struct tegra_framebuffer *to_tegra_fb(struct drm_framebuffer *fb) -{ - return container_of(fb, struct tegra_framebuffer, base); -} - struct host1x { struct drm_device *drm; struct device *dev; @@ -44,7 +34,6 @@ struct host1x { struct list_head clients;
struct drm_fbdev_cma *fbdev; - struct tegra_framebuffer fb; };
struct host1x_client;
Add support for the B and C planes which support RGB and YUV pixel formats and can be used as overlays or hardware cursor. Currently 32-bit XRGB as well as UYVY, YUV420 and YUV422 pixel formats are advertised. Other formats should be easy to add but these are the most common ones and should cover the majority of use-cases.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- Changes in v3: - split DC_CMD_STATE_CONTROL writes - adjust plane index to match window index - drop support for YUV422 format for now - disable active planes when CRTC is disabled - disable plane on module unload
Changes in v4: - add proper YUV420, YUV422 and UYVY support
drivers/gpu/drm/tegra/dc.c | 431 ++++++++++++++++++++++++++++++++++---------- drivers/gpu/drm/tegra/dc.h | 12 +- drivers/gpu/drm/tegra/drm.h | 24 +++ 3 files changed, 372 insertions(+), 95 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index d35ff8b..52bf63f 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -18,26 +18,143 @@ #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) +{ + struct tegra_plane *p = to_tegra_plane(plane); + struct tegra_dc *dc = to_tegra_dc(crtc); + struct tegra_dc_window window; + unsigned int i; + + 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; + + for (i = 0; i < drm_format_num_planes(fb->pixel_format); i++) { + struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, i); + + window.base[i] = gem->paddr + fb->offsets[i]; + + /* + * Tegra doesn't support different strides for U and V planes + * so we display a warning if the user tries to display a + * framebuffer with such a configuration. + */ + if (i >= 2) { + if (fb->pitches[i] != window.stride[1]) + DRM_ERROR("unsupported UV-plane configuration\n"); + } else { + window.stride[i] = fb->pitches[i]; + } + } + + return tegra_dc_setup_window(dc, p->index, &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 long value; + + value = WINDOW_A_SELECT << p->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); + + tegra_dc_writel(dc, WIN_A_UPDATE << p->index, DC_CMD_STATE_CONTROL); + tegra_dc_writel(dc, WIN_A_ACT_REQ << p->index, DC_CMD_STATE_CONTROL); + + return 0; +} + +static void tegra_plane_destroy(struct drm_plane *plane) +{ + tegra_plane_disable(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_UYVY, + DRM_FORMAT_YUV420, + 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 = 1 + 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, };
-static void tegra_crtc_dpms(struct drm_crtc *crtc, int mode) +static void tegra_crtc_disable(struct drm_crtc *crtc) { + struct drm_device *drm = crtc->dev; + struct drm_plane *plane; + + list_for_each_entry(plane, &drm->mode_config.plane_list, head) { + if (plane->crtc == crtc) { + tegra_plane_disable(plane); + plane->crtc = NULL; + + if (plane->fb) { + drm_framebuffer_unreference(plane->fb); + plane->fb = NULL; + } + } + } }
static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, @@ -47,10 +164,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 +198,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 +272,185 @@ static int tegra_crtc_setup_clk(struct drm_crtc *crtc, return 0; }
+static bool tegra_dc_format_is_yuv(unsigned int format, bool *planar) +{ + switch (format) { + case WIN_COLOR_DEPTH_YCbCr422: + case WIN_COLOR_DEPTH_YUV422: + if (planar) + *planar = false; + + return true; + + case WIN_COLOR_DEPTH_YCbCr420P: + case WIN_COLOR_DEPTH_YUV420P: + case WIN_COLOR_DEPTH_YCbCr422P: + case WIN_COLOR_DEPTH_YUV422P: + case WIN_COLOR_DEPTH_YCbCr422R: + case WIN_COLOR_DEPTH_YUV422R: + case WIN_COLOR_DEPTH_YCbCr422RA: + case WIN_COLOR_DEPTH_YUV422RA: + if (planar) + *planar = true; + + return true; + } + + return false; +} + +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; + bool yuv, planar; + + /* + * For YUV planar modes, the number of bytes per pixel takes into + * account only the luma component and therefore is 1. + */ + yuv = tegra_dc_format_is_yuv(window->format, &planar); + if (!yuv) + bpp = window->bits_per_pixel / 8; + else + bpp = planar ? 1 : 2; + + 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); + + /* + * For DDA computations the number of bytes per pixel for YUV planar + * modes needs to take into account all Y, U and V components. + */ + if (yuv && planar) + bpp = 2; + + 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[0], DC_WINBUF_START_ADDR); + + if (yuv && planar) { + tegra_dc_writel(dc, window->base[1], DC_WINBUF_START_ADDR_U); + tegra_dc_writel(dc, window->base[2], DC_WINBUF_START_ADDR_V); + value = window->stride[1] << 16 | window->stride[0]; + tegra_dc_writel(dc, value, DC_WIN_LINE_STRIDE); + } else { + tegra_dc_writel(dc, window->stride[0], 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 (yuv) { + /* setup default colorspace conversion coefficients */ + tegra_dc_writel(dc, 0x00f0, DC_WIN_CSC_YOF); + tegra_dc_writel(dc, 0x012a, DC_WIN_CSC_KYRGB); + tegra_dc_writel(dc, 0x0000, DC_WIN_CSC_KUR); + tegra_dc_writel(dc, 0x0198, DC_WIN_CSC_KVR); + tegra_dc_writel(dc, 0x039b, DC_WIN_CSC_KUG); + tegra_dc_writel(dc, 0x032f, DC_WIN_CSC_KVG); + tegra_dc_writel(dc, 0x0204, DC_WIN_CSC_KUB); + tegra_dc_writel(dc, 0x0000, DC_WIN_CSC_KVB); + + value |= CSC_ENABLE; + } else 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; + } + + tegra_dc_writel(dc, WIN_A_UPDATE << index, DC_CMD_STATE_CONTROL); + tegra_dc_writel(dc, WIN_A_ACT_REQ << index, 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; + + case DRM_FORMAT_UYVY: + return WIN_COLOR_DEPTH_YCbCr422; + + case DRM_FORMAT_YUV420: + return WIN_COLOR_DEPTH_YCbCr420P; + + case DRM_FORMAT_YUV422: + return WIN_COLOR_DEPTH_YCbCr422P; + + 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 +458,7 @@ static int tegra_crtc_mode_set(struct drm_crtc *crtc, { struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(crtc->fb, 0); 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 +489,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[0] = crtc->fb->pitches[0]; + window.base[0] = gem->paddr; + + err = tegra_dc_setup_window(dc, 0, &window); + if (err < 0) + dev_err(dc->dev, "failed to enable root plane\n");
return 0; } @@ -347,7 +586,7 @@ static void tegra_crtc_load_lut(struct drm_crtc *crtc) }
static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = { - .dpms = tegra_crtc_dpms, + .disable = tegra_crtc_disable, .mode_fixup = tegra_crtc_mode_fixup, .mode_set = tegra_crtc_mode_set, .prepare = tegra_crtc_prepare, @@ -588,7 +827,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 +929,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..e2fa328 100644 --- a/drivers/gpu/drm/tegra/dc.h +++ b/drivers/gpu/drm/tegra/dc.h @@ -290,8 +290,18 @@ #define DC_DISP_SD_HW_K_VALUES 0x4dd #define DC_DISP_SD_MAN_K_VALUES 0x4de
+#define DC_WIN_CSC_YOF 0x611 +#define DC_WIN_CSC_KYRGB 0x612 +#define DC_WIN_CSC_KUR 0x613 +#define DC_WIN_CSC_KVR 0x614 +#define DC_WIN_CSC_KUG 0x615 +#define DC_WIN_CSC_KVG 0x616 +#define DC_WIN_CSC_KUB 0x617 +#define DC_WIN_CSC_KVB 0x618 + #define DC_WIN_WIN_OPTIONS 0x700 #define COLOR_EXPAND (1 << 6) +#define CSC_ENABLE (1 << 18) #define WIN_ENABLE (1 << 30)
#define DC_WIN_BYTE_SWAP 0x701 @@ -359,7 +369,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 3c61aab..896ff43 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -107,6 +107,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[2]; + unsigned long base[3]; +}; + +/* 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 tegra_output_ops { int (*enable)(struct tegra_output *output); int (*disable)(struct tegra_output *output);
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 --- Changes in v3: - split DC_CMD_STATE_CONTROL writes
Changes in v4: - reset line stride to fix cases where the framebuffer size doesn't match the display mode (suggested by Mark Zhang)
drivers/gpu/drm/tegra/dc.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 52bf63f..ceb2f52 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -134,6 +134,29 @@ 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 drm_framebuffer *fb) +{ + struct drm_gem_cma_object *gem = drm_fb_cma_get_gem_obj(fb, 0); + unsigned long value; + + tegra_dc_writel(dc, WINDOW_A_SELECT, DC_CMD_DISPLAY_WINDOW_HEADER); + + value = fb->offsets[0] + y * fb->pitches[0] + + x * fb->bits_per_pixel / 8; + + tegra_dc_writel(dc, gem->paddr + value, DC_WINBUF_START_ADDR); + tegra_dc_writel(dc, fb->pitches[0], DC_WIN_LINE_STRIDE); + + value = GENERAL_UPDATE | WIN_A_UPDATE; + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); + + value = GENERAL_ACT_REQ | WIN_A_ACT_REQ; + 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, @@ -510,6 +533,14 @@ 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_dc *dc = to_tegra_dc(crtc); + + return tegra_dc_set_base(dc, x, y, crtc->fb); +} + static void tegra_crtc_prepare(struct drm_crtc *crtc) { struct tegra_dc *dc = to_tegra_dc(crtc); @@ -589,6 +620,7 @@ static const struct drm_crtc_helper_funcs tegra_crtc_helper_funcs = { .disable = tegra_crtc_disable, .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 --- Changes in v3: - implement dummy .get_vblank_counter()
drivers/gpu/drm/tegra/dc.c | 55 +++++++++++++++++++++++++++++++-------------- drivers/gpu/drm/tegra/drm.c | 50 +++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/drm.h | 3 +++ 3 files changed, 91 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index ceb2f52..5f55a25 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -157,6 +157,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, @@ -485,6 +511,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); @@ -585,31 +613,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) @@ -626,7 +646,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; @@ -971,7 +991,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, @@ -1020,6 +1040,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..4e31dac 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,48 @@ 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 u32 tegra_drm_get_vblank_counter(struct drm_device *dev, int crtc) +{ + /* TODO: implement real hardware counter using syncpoints */ + return drm_vblank_count(dev, crtc); +} + +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 +142,10 @@ struct drm_driver tegra_drm_driver = { .open = tegra_drm_open, .lastclose = tegra_drm_lastclose,
+ .get_vblank_counter = tegra_drm_get_vblank_counter, + .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 896ff43..01f0aee 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -64,6 +64,7 @@ struct tegra_output;
struct tegra_dc { struct host1x_client client; + spinlock_t lock;
struct host1x *host1x; struct device *dev; @@ -130,6 +131,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 tegra_output_ops { int (*enable)(struct tegra_output *output);
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 --- Changes in v3: - use drm_send_vblank_event() - set crtc->fb field
Changes in v4: - fix a potential race by checking that a framebuffer base has been latched when completing a page-flip
drivers/gpu/drm/tegra/dc.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/dc.h | 2 ++ drivers/gpu/drm/tegra/drm.c | 9 +++++++ drivers/gpu/drm/tegra/drm.h | 5 ++++ 4 files changed, 82 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 5f55a25..c5d825f 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev; + struct drm_crtc *crtc = &dc->base; + struct drm_gem_cma_object *gem; + unsigned long flags, base; + + if (!dc->event) + return; + + gem = drm_fb_cma_get_gem_obj(crtc->fb, 0); + + /* check if new start address has been latched */ + tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS); + base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR); + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS); + + if (base == gem->paddr + crtc->fb->offsets[0]) { + spin_lock_irqsave(&drm->event_lock, flags); + drm_send_vblank_event(drm, dc->pipe, dc->event); + drm_vblank_put(drm, dc->pipe); + dc->event = NULL; + spin_unlock_irqrestore(&drm->event_lock, flags); + } +} + +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_dc *dc = to_tegra_dc(crtc); + struct drm_device *drm = crtc->dev; + + if (dc->event) + return -EBUSY; + + if (event) { + event->pipe = dc->pipe; + dc->event = event; + drm_vblank_get(drm, dc->pipe); + } + + tegra_dc_set_base(dc, 0, 0, fb); + crtc->fb = fb; + + 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, }; @@ -665,6 +730,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/dc.h b/drivers/gpu/drm/tegra/dc.h index e2fa328..79eaec9 100644 --- a/drivers/gpu/drm/tegra/dc.h +++ b/drivers/gpu/drm/tegra/dc.h @@ -58,6 +58,8 @@ #define DC_CMD_SIGNAL_RAISE3 0x03e
#define DC_CMD_STATE_ACCESS 0x040 +#define READ_MUX (1 << 0) +#define WRITE_MUX (1 << 2)
#define DC_CMD_STATE_CONTROL 0x041 #define GENERAL_ACT_REQ (1 << 0) diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 4e31dac..97485af 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -135,11 +135,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 = tegra_drm_get_vblank_counter, diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 01f0aee..6dd75a2 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -84,6 +84,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) @@ -133,6 +136,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 tegra_output_ops { int (*enable)(struct tegra_output *output);
On 02/21/2013 03:35 PM, Thierry Reding wrote:
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
Changes in v3:
- use drm_send_vblank_event()
- set crtc->fb field
Changes in v4:
fix a potential race by checking that a framebuffer base has been latched when completing a page-flip
drivers/gpu/drm/tegra/dc.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/dc.h | 2 ++ drivers/gpu/drm/tegra/drm.c | 9 +++++++ drivers/gpu/drm/tegra/drm.h | 5 ++++ 4 files changed, 82 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 5f55a25..c5d825f 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
- struct drm_crtc *crtc = &dc->base;
- struct drm_gem_cma_object *gem;
- unsigned long flags, base;
The checks for properly latched scanout look good to me and should fix the race between software and hardware. But shouldn't you already spin_lock_irqsave() here, before checking dc->event and performing write access on the DC_CMD_STATE_ACCESS registers? And lock around most of the tegra_dc_page_flip() code below, like in your previous iteration of the patch, to prevent a race between pageflip ioctl and the vblank irq handler?
- if (!dc->event)
-> !dc->event may exit prematurely because the irq handler on one core doesn't notice the new setup of dc->event by tegra_dc_page_flip() on a different core? Don't know if that can happen, don't know the memory ordering of arm, but could imagine that this or the compiler reordering instructions could cause trouble without lock protection.
return;- gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
-> crtc->fb gets updated in tegra_dc_page_flip() after tegra_dc_set_base() without lock -> possibly stale fb here?
- /* check if new start address has been latched */
- tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
- base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
- tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
-> Can other code in the driver write to DC_CMD_STATE_ACCESS, simultaneously to tegra_dc_page_flip writing->reading->writing and cause trouble?
- if (base == gem->paddr + crtc->fb->offsets[0]) {
spin_lock_irqsave(&drm->event_lock, flags);drm_send_vblank_event(drm, dc->pipe, dc->event);drm_vblank_put(drm, dc->pipe);dc->event = NULL;spin_unlock_irqrestore(&drm->event_lock, flags);- }
+}
+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_dc *dc = to_tegra_dc(crtc);
- struct drm_device *drm = crtc->dev;
- if (dc->event)
return -EBUSY;
-> Lock already here?
- if (event) {
event->pipe = dc->pipe;dc->event = event;drm_vblank_get(drm, dc->pipe);- }
- tegra_dc_set_base(dc, 0, 0, fb);
- crtc->fb = fb;
-> Unlock here?
- return 0;
+}
thanks, -mario
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, };
@@ -665,6 +730,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/dc.h b/drivers/gpu/drm/tegra/dc.h index e2fa328..79eaec9 100644 --- a/drivers/gpu/drm/tegra/dc.h +++ b/drivers/gpu/drm/tegra/dc.h @@ -58,6 +58,8 @@ #define DC_CMD_SIGNAL_RAISE3 0x03e
#define DC_CMD_STATE_ACCESS 0x040 +#define READ_MUX (1 << 0) +#define WRITE_MUX (1 << 2)
#define DC_CMD_STATE_CONTROL 0x041 #define GENERAL_ACT_REQ (1 << 0) diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 4e31dac..97485af 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -135,11 +135,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 = tegra_drm_get_vblank_counter,
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 01f0aee..6dd75a2 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -84,6 +84,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)
@@ -133,6 +136,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 tegra_output_ops { int (*enable)(struct tegra_output *output);
On Fri, Feb 22, 2013 at 05:13:47PM +0100, Mario Kleiner wrote:
On 02/21/2013 03:35 PM, Thierry Reding wrote:
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
Changes in v3:
- use drm_send_vblank_event()
- set crtc->fb field
Changes in v4:
- fix a potential race by checking that a framebuffer base has been latched when completing a page-flip
drivers/gpu/drm/tegra/dc.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/dc.h | 2 ++ drivers/gpu/drm/tegra/drm.c | 9 +++++++ drivers/gpu/drm/tegra/drm.h | 5 ++++ 4 files changed, 82 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 5f55a25..c5d825f 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
- struct drm_crtc *crtc = &dc->base;
- struct drm_gem_cma_object *gem;
- unsigned long flags, base;
The checks for properly latched scanout look good to me and should fix the race between software and hardware. But shouldn't you already spin_lock_irqsave() here, before checking dc->event and performing write access on the DC_CMD_STATE_ACCESS registers? And lock around most of the tegra_dc_page_flip() code below, like in your previous iteration of the patch, to prevent a race between pageflip ioctl and the vblank irq handler?
Currently there's nothing else that touches the DC_CMD_STATE_ACCESS register, so that part should be safe.
As to the locking that I removed since the previous patch, I looked at it more closely and didn't think it was necessary. Also nothing in my tests seemed to point to any race here, though I probably didn't cover everything.
- if (!dc->event)
-> !dc->event may exit prematurely because the irq handler on one core doesn't notice the new setup of dc->event by tegra_dc_page_flip() on a different core? Don't know if that can happen, don't know the memory ordering of arm, but could imagine that this or the compiler reordering instructions could cause trouble without lock protection.
I'm not sure I follow here. If the handler sees a NULL here, why would it want to continue? It can safely assume that the page flip wasn't setup for this interval, can't it?
If indeed the VBLANK interrupt happens exactly around the assignment to dc->event, then we'll just schedule the page-flip for the next VBLANK.
return;- gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
-> crtc->fb gets updated in tegra_dc_page_flip() after tegra_dc_set_base() without lock -> possibly stale fb here?
Good point. That may indeed require a lock. I'll need to look more closely.
- /* check if new start address has been latched */
- tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
- base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
- tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
-> Can other code in the driver write to DC_CMD_STATE_ACCESS, simultaneously to tegra_dc_page_flip writing->reading->writing and cause trouble?
This is the only location where the DC_CMD_STATE_ACCESS register is actually used in the driver so we should be safe for now.
+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event)+{
- struct tegra_dc *dc = to_tegra_dc(crtc);
- struct drm_device *drm = crtc->dev;
- if (dc->event)
return -EBUSY;-> Lock already here?
- if (event) {
event->pipe = dc->pipe;dc->event = event;drm_vblank_get(drm, dc->pipe);- }
- tegra_dc_set_base(dc, 0, 0, fb);
- crtc->fb = fb;
-> Unlock here?
I tried to expand the lock after we discussed this previously but it caused the code to deadlock. While in the process of tracking it down I decided that the lock wasn't actually required at all.
But I hadn't thought about the stale FB issue, so I'll check again. I notice that Dave has already merged this series, so if nobody objects I'll fix it up in a follow-up patch.
Thierry
On 02/25/2013 08:06 AM, Thierry Reding wrote:
On Fri, Feb 22, 2013 at 05:13:47PM +0100, Mario Kleiner wrote:
On 02/21/2013 03:35 PM, Thierry Reding wrote:
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
Changes in v3:
- use drm_send_vblank_event()
- set crtc->fb field
Changes in v4:
fix a potential race by checking that a framebuffer base has been latched when completing a page-flip
drivers/gpu/drm/tegra/dc.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/dc.h | 2 ++ drivers/gpu/drm/tegra/drm.c | 9 +++++++ drivers/gpu/drm/tegra/drm.h | 5 ++++ 4 files changed, 82 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 5f55a25..c5d825f 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -183,7 +183,72 @@ 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_device *drm = dc->base.dev;
- struct drm_crtc *crtc = &dc->base;
- struct drm_gem_cma_object *gem;
- unsigned long flags, base;
The checks for properly latched scanout look good to me and should fix the race between software and hardware. But shouldn't you already spin_lock_irqsave() here, before checking dc->event and performing write access on the DC_CMD_STATE_ACCESS registers? And lock around most of the tegra_dc_page_flip() code below, like in your previous iteration of the patch, to prevent a race between pageflip ioctl and the vblank irq handler?
Currently there's nothing else that touches the DC_CMD_STATE_ACCESS register, so that part should be safe.
ok.
As to the locking that I removed since the previous patch, I looked at it more closely and didn't think it was necessary. Also nothing in my tests seemed to point to any race here, though I probably didn't cover everything.
- if (!dc->event)
-> !dc->event may exit prematurely because the irq handler on one core doesn't notice the new setup of dc->event by tegra_dc_page_flip() on a different core? Don't know if that can happen, don't know the memory ordering of arm, but could imagine that this or the compiler reordering instructions could cause trouble without lock protection.
I'm not sure I follow here. If the handler sees a NULL here, why would it want to continue? It can safely assume that the page flip wasn't setup for this interval, can't it?
If indeed the VBLANK interrupt happens exactly around the assignment to dc->event, then we'll just schedule the page-flip for the next VBLANK.
I meant the other way round. dc->event is initially NULL. Then pageflip ioctl is called, close to vblank, the code e.g., executing on core 1. if (event) branch in tegra_dc_page_flip() executes, assigns dc->event = event etc. calls tegra_dc_set_base() before onset of vblank, pageflip completes in vblank, but the irq handler and thereby tegra_dc_finish_page_flip() running on a different core doesn't notice dc->event is non-NULL, because the changes haven't propagated to the different core without some memory write barrier etc., thereby doesn't run the flip completion in the vblank of actual flip completion. But i now think my point is probably pointless, because the successive calls to drm_vblank_get() and tegra_dc_set_base() contain enough locking and implicit memory barriers that such a stale value there won't happen.
return;- gem = drm_fb_cma_get_gem_obj(crtc->fb, 0);
-> crtc->fb gets updated in tegra_dc_page_flip() after tegra_dc_set_base() without lock -> possibly stale fb here?
Good point. That may indeed require a lock. I'll need to look more closely.
Yep, i think here my argument from above may hold for crtc->fb, nothing here to prevent a race afaics.
- /* check if new start address has been latched */
- tegra_dc_writel(dc, READ_MUX, DC_CMD_STATE_ACCESS);
- base = tegra_dc_readl(dc, DC_WINBUF_START_ADDR);
- tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
-> Can other code in the driver write to DC_CMD_STATE_ACCESS, simultaneously to tegra_dc_page_flip writing->reading->writing and cause trouble?
This is the only location where the DC_CMD_STATE_ACCESS register is actually used in the driver so we should be safe for now.
Ok. I'm just paranoid about forgetting such things on later changes.
+static int tegra_dc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,
struct drm_pending_vblank_event *event)+{
- struct tegra_dc *dc = to_tegra_dc(crtc);
- struct drm_device *drm = crtc->dev;
- if (dc->event)
return -EBUSY;-> Lock already here?
- if (event) {
event->pipe = dc->pipe;dc->event = event;drm_vblank_get(drm, dc->pipe);- }
- tegra_dc_set_base(dc, 0, 0, fb);
- crtc->fb = fb;
-> Unlock here?
I tried to expand the lock after we discussed this previously but it caused the code to deadlock. While in the process of tracking it down I decided that the lock wasn't actually required at all.
But I hadn't thought about the stale FB issue, so I'll check again. I notice that Dave has already merged this series, so if nobody objects I'll fix it up in a follow-up patch.
Fine with me.
thanks, -mario
The Tegra TRM says that the ACT_REQ and UPDATE fields cannot be programmed at the same time so they are updated in two consecutive writes instead.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index c5d825f..2086400 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -689,9 +689,10 @@ static void tegra_crtc_commit(struct drm_crtc *crtc) struct tegra_dc *dc = to_tegra_dc(crtc); unsigned long value;
- value = GENERAL_ACT_REQ | WIN_A_ACT_REQ | - GENERAL_UPDATE | WIN_A_UPDATE; + value = GENERAL_UPDATE | WIN_A_UPDATE; + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
+ value = GENERAL_ACT_REQ | WIN_A_ACT_REQ; tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
drm_vblank_post_modeset(crtc->dev, dc->pipe);
bpp stores the number of bytes per pixel, but color expansion needs to be enabled for less than 24 bits per pixel.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/dc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 2086400..bf8095c 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -500,7 +500,7 @@ int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index, tegra_dc_writel(dc, 0x0000, DC_WIN_CSC_KVB);
value |= CSC_ENABLE; - } else if (bpp < 24) { + } else if (window->bits_per_pixel < 24) { value |= COLOR_EXPAND; }
This list is most useful to inspect whether framebuffer reference counting works as expected. The code is loosely based on the i915 implementation.
Signed-off-by: Thierry Reding thierry.reding@avionic-design.de --- drivers/gpu/drm/tegra/drm.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 97485af..181a370 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -143,6 +143,45 @@ static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) tegra_dc_cancel_page_flip(crtc, file); }
+#ifdef CONFIG_DEBUG_FS +static int tegra_debugfs_framebuffers(struct seq_file *s, void *data) +{ + struct drm_info_node *node = (struct drm_info_node *)s->private; + struct drm_device *drm = node->minor->dev; + struct drm_framebuffer *fb; + + mutex_lock(&drm->mode_config.fb_lock); + + list_for_each_entry(fb, &drm->mode_config.fb_list, head) { + seq_printf(s, "%3d: user size: %d x %d, depth %d, %d bpp, refcount %d\n", + fb->base.id, fb->width, fb->height, fb->depth, + fb->bits_per_pixel, + atomic_read(&fb->refcount.refcount)); + } + + mutex_unlock(&drm->mode_config.fb_lock); + + return 0; +} + +static struct drm_info_list tegra_debugfs_list[] = { + { "framebuffers", tegra_debugfs_framebuffers, 0 }, +}; + +static int tegra_debugfs_init(struct drm_minor *minor) +{ + return drm_debugfs_create_files(tegra_debugfs_list, + ARRAY_SIZE(tegra_debugfs_list), + minor->debugfs_root, minor); +} + +static void tegra_debugfs_cleanup(struct drm_minor *minor) +{ + drm_debugfs_remove_files(tegra_debugfs_list, + ARRAY_SIZE(tegra_debugfs_list), minor); +} +#endif + struct drm_driver tegra_drm_driver = { .driver_features = DRIVER_BUS_PLATFORM | DRIVER_MODESET | DRIVER_GEM, .load = tegra_drm_load, @@ -155,6 +194,11 @@ struct drm_driver tegra_drm_driver = { .enable_vblank = tegra_drm_enable_vblank, .disable_vblank = tegra_drm_disable_vblank,
+#if defined(CONFIG_DEBUG_FS) + .debugfs_init = tegra_debugfs_init, + .debugfs_cleanup = tegra_debugfs_cleanup, +#endif + .gem_free_object = drm_gem_cma_free_object, .gem_vm_ops = &drm_gem_cma_vm_ops, .dumb_create = drm_gem_cma_dumb_create,
dri-devel@lists.freedesktop.org
-
Mario Kleiner -
Mario Kleiner -
Thierry Reding