[PATCH v4 15/22] drm/tilcdc: Get rid of complex ping-pong mechanism
Rob Clark
robdclark at gmail.com
Wed Feb 24 20:31:34 UTC 2016
On Wed, Feb 24, 2016 at 11:48 AM, Jyri Sarha <jsarha at ti.com> wrote:
> From: Tomi Valkeinen <tomi.valkeinen at ti.com>
>
> Get rid of complex ping-pong mechanism and replace it with simpler
> single buffer flipping code.
>
> The LCDC HW appears to be designed mainly static framebuffers in
> mind. There are two modes of operation, either static single buffer,
> or ping pong double buffering with two static buffers switching back
> and forth. Luckily the framebuffer start address is fetched only in
> the beginning of the frame and changing the address after that only
> takes effect after the next vertical blank. The page flipping code can
> simply write the address of the new framebuffer and the page is
> flipped automatically after the next vertical blank. Using the ping
> pong double buffering makes the flipping code way more complex and it
> does not provide any benefit, so it is better to switch to single
> buffer operation.
>
> There is still one problem in updating the framebuffer dma address on
> the fly. There are two registers defining the framebuffer dma area and
> things may break if the dma address is fetched in while the registers
> are are being updated.
Just curious, but does this mean you need to avoid writing the reg's
too near to vblank? (I think there are other drivers which do not
have double buffered registers and must do this.. but it's a pain..)
The ping-pong mode is super-wonky, and would be nice to get rid of..
but with this scheme I'm not quite sure what happens if a pageflip
comes in too close to vblank..
BR,
-R
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ti.com>
> [Added description to the patch]
> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> ---
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 121 +++++++++++++++--------------------
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 27 +-------
> 2 files changed, 53 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index a6ef737..32572285 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -25,15 +25,12 @@ struct tilcdc_crtc {
> struct drm_crtc base;
>
> const struct tilcdc_panel_info *info;
> - uint32_t dirty;
> - dma_addr_t start, end;
> struct drm_pending_vblank_event *event;
> int dpms;
> wait_queue_head_t frame_done_wq;
> bool frame_done;
>
> - /* fb currently set to scanout 0/1: */
> - struct drm_framebuffer *scanout[2];
> + struct drm_framebuffer *curr_fb;
>
> /* for deferred fb unref's: */
> struct drm_flip_work unref_work;
> @@ -54,62 +51,31 @@ static void unref_worker(struct drm_flip_work *work, void *val)
> mutex_unlock(&dev->mode_config.mutex);
> }
>
> -static void set_scanout(struct drm_crtc *crtc, int n)
> -{
> - static const uint32_t base_reg[] = {
> - LCDC_DMA_FB_BASE_ADDR_0_REG,
> - LCDC_DMA_FB_BASE_ADDR_1_REG,
> - };
> - static const uint32_t ceil_reg[] = {
> - LCDC_DMA_FB_CEILING_ADDR_0_REG,
> - LCDC_DMA_FB_CEILING_ADDR_1_REG,
> - };
> - static const uint32_t stat[] = {
> - LCDC_END_OF_FRAME0, LCDC_END_OF_FRAME1,
> - };
> - struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> - struct drm_device *dev = crtc->dev;
> - struct tilcdc_drm_private *priv = dev->dev_private;
> -
> - tilcdc_write(dev, base_reg[n], tilcdc_crtc->start);
> - tilcdc_write(dev, ceil_reg[n], tilcdc_crtc->end);
> - if (tilcdc_crtc->scanout[n]) {
> - drm_flip_work_queue(&tilcdc_crtc->unref_work, tilcdc_crtc->scanout[n]);
> - drm_flip_work_commit(&tilcdc_crtc->unref_work, priv->wq);
> - }
> - tilcdc_crtc->scanout[n] = crtc->primary->fb;
> - drm_framebuffer_reference(tilcdc_crtc->scanout[n]);
> - tilcdc_crtc->dirty &= ~stat[n];
> -}
> -
> -static void update_scanout(struct drm_crtc *crtc)
> +static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
> {
> struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> struct drm_device *dev = crtc->dev;
> - struct drm_framebuffer *fb = crtc->primary->fb;
> struct drm_gem_cma_object *gem;
> unsigned int depth, bpp;
> + dma_addr_t start, end;
>
> drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> gem = drm_fb_cma_get_gem_obj(fb, 0);
>
> - tilcdc_crtc->start = gem->paddr + fb->offsets[0] +
> - (crtc->y * fb->pitches[0]) + (crtc->x * bpp/8);
> + start = gem->paddr + fb->offsets[0] +
> + crtc->y * fb->pitches[0] +
> + crtc->x * bpp / 8;
>
> - tilcdc_crtc->end = tilcdc_crtc->start +
> - (crtc->mode.vdisplay * fb->pitches[0]);
> + end = start + (crtc->mode.vdisplay * fb->pitches[0]);
>
> - if (tilcdc_crtc->dpms == DRM_MODE_DPMS_ON) {
> - /* already enabled, so just mark the frames that need
> - * updating and they will be updated on vblank:
> - */
> - tilcdc_crtc->dirty |= LCDC_END_OF_FRAME0 | LCDC_END_OF_FRAME1;
> - drm_vblank_get(dev, 0);
> - } else {
> - /* not enabled yet, so update registers immediately: */
> - set_scanout(crtc, 0);
> - set_scanout(crtc, 1);
> - }
> + tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, start);
> + tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, end);
> +
> + if (tilcdc_crtc->curr_fb)
> + drm_flip_work_queue(&tilcdc_crtc->unref_work,
> + tilcdc_crtc->curr_fb);
> +
> + tilcdc_crtc->curr_fb = fb;
> }
>
> static void reset(struct drm_crtc *crtc)
> @@ -131,7 +97,7 @@ static void start(struct drm_crtc *crtc)
>
> reset(crtc);
>
> - tilcdc_set(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE);
> + tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE);
> tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_PALETTE_LOAD_MODE(DATA_ONLY));
> tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
> }
> @@ -179,6 +145,7 @@ static int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
> struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> struct drm_device *dev = crtc->dev;
> int r;
> + unsigned long flags;
>
> r = tilcdc_verify_fb(crtc, fb);
> if (r)
> @@ -189,12 +156,18 @@ static int tilcdc_crtc_page_flip(struct drm_crtc *crtc,
> return -EBUSY;
> }
>
> + drm_framebuffer_reference(fb);
> +
> crtc->primary->fb = fb;
> - tilcdc_crtc->event = event;
>
> pm_runtime_get_sync(dev->dev);
>
> - update_scanout(crtc);
> +
> + set_scanout(crtc, fb);
> +
> + spin_lock_irqsave(&dev->event_lock, flags);
> + tilcdc_crtc->event = event;
> + spin_unlock_irqrestore(&dev->event_lock, flags);
>
> pm_runtime_put_sync(dev->dev);
>
> @@ -237,6 +210,14 @@ void tilcdc_crtc_dpms(struct drm_crtc *crtc, int mode)
> }
>
> pm_runtime_put_sync(dev->dev);
> +
> + if (tilcdc_crtc->curr_fb) {
> + drm_flip_work_queue(&tilcdc_crtc->unref_work,
> + tilcdc_crtc->curr_fb);
> + tilcdc_crtc->curr_fb = NULL;
> + }
> +
> + drm_flip_work_commit(&tilcdc_crtc->unref_work, priv->wq);
> }
> }
>
> @@ -450,8 +431,10 @@ static int tilcdc_crtc_mode_set(struct drm_crtc *crtc,
> else
> tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ORDER);
>
> + drm_framebuffer_reference(crtc->primary->fb);
> +
> + set_scanout(crtc, crtc->primary->fb);
>
> - update_scanout(crtc);
> tilcdc_crtc_update_clk(crtc);
>
> pm_runtime_put_sync(dev->dev);
> @@ -469,9 +452,14 @@ static int tilcdc_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
> if (r)
> return r;
>
> + drm_framebuffer_reference(crtc->primary->fb);
> +
> pm_runtime_get_sync(dev->dev);
> - update_scanout(crtc);
> +
> + set_scanout(crtc, crtc->primary->fb);
> +
> pm_runtime_put_sync(dev->dev);
> +
> return 0;
> }
>
> @@ -661,30 +649,21 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
> stat = tilcdc_read_irqstatus(dev);
> tilcdc_clear_irqstatus(dev, stat);
>
> - if ((stat & LCDC_END_OF_FRAME0) || (stat & LCDC_END_OF_FRAME1)) {
> - struct drm_pending_vblank_event *event;
> + if (stat & LCDC_END_OF_FRAME0) {
> unsigned long flags;
> - uint32_t dirty = tilcdc_crtc->dirty & stat;
> -
> - tilcdc_clear_irqstatus(dev, stat);
>
> - if (dirty & LCDC_END_OF_FRAME0)
> - set_scanout(crtc, 0);
> -
> - if (dirty & LCDC_END_OF_FRAME1)
> - set_scanout(crtc, 1);
> + drm_flip_work_commit(&tilcdc_crtc->unref_work, priv->wq);
>
> drm_handle_vblank(dev, 0);
>
> spin_lock_irqsave(&dev->event_lock, flags);
> - event = tilcdc_crtc->event;
> - tilcdc_crtc->event = NULL;
> - if (event)
> - drm_send_vblank_event(dev, 0, event);
> - spin_unlock_irqrestore(&dev->event_lock, flags);
>
> - if (dirty && !tilcdc_crtc->dirty)
> - drm_vblank_put(dev, 0);
> + if (tilcdc_crtc->event) {
> + drm_send_vblank_event(dev, 0, tilcdc_crtc->event);
> + tilcdc_crtc->event = NULL;
> + }
> +
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> if (priv->rev == 2) {
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index e1f6979..c5d9e3a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -381,6 +381,7 @@ static int tilcdc_irq_postinstall(struct drm_device *dev)
> else
> tilcdc_set(dev, LCDC_INT_ENABLE_SET_REG,
> LCDC_V2_UNDERFLOW_INT_ENA |
> + LCDC_V2_END_OF_FRAME0_INT_ENA |
> LCDC_FRAME_DONE);
>
> return 0;
> @@ -398,41 +399,19 @@ static void tilcdc_irq_uninstall(struct drm_device *dev)
> } else {
> tilcdc_clear(dev, LCDC_INT_ENABLE_SET_REG,
> LCDC_V2_UNDERFLOW_INT_ENA | LCDC_V2_PL_INT_ENA |
> - LCDC_V2_END_OF_FRAME0_INT_ENA | LCDC_V2_END_OF_FRAME1_INT_ENA |
> + LCDC_V2_END_OF_FRAME0_INT_ENA |
> LCDC_FRAME_DONE);
> }
> -
> -}
> -
> -static void enable_vblank(struct drm_device *dev, bool enable)
> -{
> - struct tilcdc_drm_private *priv = dev->dev_private;
> - u32 reg, mask;
> -
> - if (priv->rev == 1) {
> - reg = LCDC_DMA_CTRL_REG;
> - mask = LCDC_V1_END_OF_FRAME_INT_ENA;
> - } else {
> - reg = LCDC_INT_ENABLE_SET_REG;
> - mask = LCDC_V2_END_OF_FRAME0_INT_ENA |
> - LCDC_V2_END_OF_FRAME1_INT_ENA;
> - }
> -
> - if (enable)
> - tilcdc_set(dev, reg, mask);
> - else
> - tilcdc_clear(dev, reg, mask);
> }
>
> static int tilcdc_enable_vblank(struct drm_device *dev, unsigned int pipe)
> {
> - enable_vblank(dev, true);
> return 0;
> }
>
> static void tilcdc_disable_vblank(struct drm_device *dev, unsigned int pipe)
> {
> - enable_vblank(dev, false);
> + return;
> }
>
> #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_PM_SLEEP)
> --
> 1.9.1
>
More information about the dri-devel
mailing list