[PATCH] drm: hdlcd: Replace the mode config's atomic_commit with the proper implementation.
Daniel Vetter
daniel at ffwll.ch
Tue May 31 11:18:06 UTC 2016
On Tue, May 31, 2016 at 12:03:04PM +0100, Liviu Dudau wrote:
> The HDLCD atomic_commit code was mis-using the drm_atomic_helper_commit()
> function to implement the non-blocking code. Replace that with the correct
> implementation that does the async queue-ing of commits.
>
> Signed-off-by: Liviu Dudau <Liviu.Dudau at arm.com>
Please don't roll your own atomic nonblocking commit any more, but instead
use the generic one I'm working on:
https://cgit.freedesktop.org/~danvet/drm/log/
Review and testing very much appreciated (the patches are on the m-l,
too).
Cheers, Daniel
> ---
> drivers/gpu/drm/arm/hdlcd_crtc.c | 64 +++++++++++++++++++---------
> drivers/gpu/drm/arm/hdlcd_drv.c | 90 ++++++++++++++++++++++++++++------------
> drivers/gpu/drm/arm/hdlcd_drv.h | 3 +-
> 3 files changed, 109 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index d1e8d31..523890d 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -106,7 +106,7 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc)
> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> struct drm_display_mode *m = &crtc->state->adjusted_mode;
> struct videomode vm;
> - unsigned int polarities, line_length, err;
> + unsigned int polarities, err;
>
> vm.vfront_porch = m->crtc_vsync_start - m->crtc_vdisplay;
> vm.vback_porch = m->crtc_vtotal - m->crtc_vsync_end;
> @@ -122,23 +122,18 @@ static void hdlcd_crtc_mode_set_nofb(struct drm_crtc *crtc)
> if (m->flags & DRM_MODE_FLAG_PVSYNC)
> polarities |= HDLCD_POLARITY_VSYNC;
>
> - line_length = crtc->primary->state->fb->pitches[0];
> -
> /* Allow max number of outstanding requests and largest burst size */
> hdlcd_write(hdlcd, HDLCD_REG_BUS_OPTIONS,
> HDLCD_BUS_MAX_OUTSTAND | HDLCD_BUS_BURST_16);
>
> - hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, line_length);
> - hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, line_length);
> - hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, m->crtc_vdisplay - 1);
> hdlcd_write(hdlcd, HDLCD_REG_V_DATA, m->crtc_vdisplay - 1);
> hdlcd_write(hdlcd, HDLCD_REG_V_BACK_PORCH, vm.vback_porch - 1);
> hdlcd_write(hdlcd, HDLCD_REG_V_FRONT_PORCH, vm.vfront_porch - 1);
> hdlcd_write(hdlcd, HDLCD_REG_V_SYNC, vm.vsync_len - 1);
> + hdlcd_write(hdlcd, HDLCD_REG_H_DATA, m->crtc_hdisplay - 1);
> hdlcd_write(hdlcd, HDLCD_REG_H_BACK_PORCH, vm.hback_porch - 1);
> hdlcd_write(hdlcd, HDLCD_REG_H_FRONT_PORCH, vm.hfront_porch - 1);
> hdlcd_write(hdlcd, HDLCD_REG_H_SYNC, vm.hsync_len - 1);
> - hdlcd_write(hdlcd, HDLCD_REG_H_DATA, m->crtc_hdisplay - 1);
> hdlcd_write(hdlcd, HDLCD_REG_POLARITIES, polarities);
>
> err = hdlcd_set_pxl_fmt(crtc);
> @@ -153,20 +148,19 @@ static void hdlcd_crtc_enable(struct drm_crtc *crtc)
> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
>
> clk_prepare_enable(hdlcd->clk);
> + hdlcd_crtc_mode_set_nofb(crtc);
> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
> - drm_crtc_vblank_on(crtc);
> }
>
> static void hdlcd_crtc_disable(struct drm_crtc *crtc)
> {
> struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
>
> - if (!crtc->primary->fb)
> + if (!crtc->state->active)
> return;
>
> hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
> clk_disable_unprepare(hdlcd->clk);
> - drm_crtc_vblank_off(crtc);
> }
>
> static int hdlcd_crtc_atomic_check(struct drm_crtc *crtc,
> @@ -208,6 +202,21 @@ static void hdlcd_crtc_atomic_begin(struct drm_crtc *crtc,
> static void hdlcd_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *state)
> {
> + struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
> + struct drm_device *drm = crtc->dev;
> + unsigned long flags;
> + struct drm_pending_vblank_event *e;
> +
> + spin_lock_irqsave(&drm->event_lock, flags);
> + e = list_first_entry_or_null(&hdlcd->event_list, typeof(*e), base.link);
> +
> + if (e) {
> + list_del(&e->base.link);
> + drm_crtc_send_vblank_event(&hdlcd->crtc, e);
> + drm_crtc_vblank_put(&hdlcd->crtc);
> + }
> +
> + spin_unlock_irqrestore(&drm->event_lock, flags);
> }
>
> static bool hdlcd_crtc_mode_fixup(struct drm_crtc *crtc,
> @@ -219,13 +228,8 @@ static bool hdlcd_crtc_mode_fixup(struct drm_crtc *crtc,
>
> static const struct drm_crtc_helper_funcs hdlcd_crtc_helper_funcs = {
> .mode_fixup = hdlcd_crtc_mode_fixup,
> - .mode_set = drm_helper_crtc_mode_set,
> - .mode_set_base = drm_helper_crtc_mode_set_base,
> - .mode_set_nofb = hdlcd_crtc_mode_set_nofb,
> .enable = hdlcd_crtc_enable,
> .disable = hdlcd_crtc_disable,
> - .prepare = hdlcd_crtc_disable,
> - .commit = hdlcd_crtc_enable,
> .atomic_check = hdlcd_crtc_atomic_check,
> .atomic_begin = hdlcd_crtc_atomic_begin,
> .atomic_flush = hdlcd_crtc_atomic_flush,
> @@ -234,6 +238,15 @@ static const struct drm_crtc_helper_funcs hdlcd_crtc_helper_funcs = {
> static int hdlcd_plane_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *state)
> {
> + u32 src_w, src_h;
> +
> + src_w = state->src_w >> 16;
> + src_h = state->src_h >> 16;
> +
> + /* we can't do any scaling of the plane source */
> + if ((src_w != state->crtc_w) || (src_h != state->crtc_h))
> + return -EINVAL;
> +
> return 0;
> }
>
> @@ -242,20 +255,31 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
> {
> struct hdlcd_drm_private *hdlcd;
> struct drm_gem_cma_object *gem;
> + unsigned int depth, bpp;
> + u32 src_w, src_h, dest_w, dest_h;
> dma_addr_t scanout_start;
>
> - if (!plane->state->crtc || !plane->state->fb)
> + if (!plane->state->fb)
> return;
>
> - hdlcd = crtc_to_hdlcd_priv(plane->state->crtc);
> + drm_fb_get_bpp_depth(plane->state->fb->pixel_format, &depth, &bpp);
> + src_w = plane->state->src_w >> 16;
> + src_h = plane->state->src_h >> 16;
> + dest_w = plane->state->crtc_w;
> + dest_h = plane->state->crtc_h;
> gem = drm_fb_cma_get_gem_obj(plane->state->fb, 0);
> - scanout_start = gem->paddr;
> + scanout_start = gem->paddr + plane->state->fb->offsets[0] +
> + plane->state->crtc_y * plane->state->fb->pitches[0] +
> + plane->state->crtc_x * bpp / 8;
> +
> + hdlcd = plane->dev->dev_private;
> + hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, plane->state->fb->pitches[0]);
> + hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, plane->state->fb->pitches[0]);
> + hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
> hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
> }
>
> static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
> - .prepare_fb = NULL,
> - .cleanup_fb = NULL,
> .atomic_check = hdlcd_plane_atomic_check,
> .atomic_update = hdlcd_plane_atomic_update,
> };
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 21b1427..4585aa3 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -14,11 +14,14 @@
> #include <linux/clk.h>
> #include <linux/component.h>
> #include <linux/list.h>
> +#include <linux/mutex.h>
> #include <linux/of_graph.h>
> #include <linux/of_reserved_mem.h>
> #include <linux/pm_runtime.h>
> +#include <linux/wait.h>
>
> #include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_crtc_helper.h>
> @@ -108,10 +111,60 @@ static void hdlcd_fb_output_poll_changed(struct drm_device *drm)
> drm_fbdev_cma_hotplug_event(hdlcd->fbdev);
> }
>
> +static void hdlcd_atomic_complete(struct drm_device *dev,
> + struct drm_atomic_state *state)
> +{
> + /* ToDo: Wait for fences */
> + drm_atomic_helper_commit_modeset_disables(dev, state);
> + drm_atomic_helper_commit_planes(dev, state, false);
> + drm_atomic_helper_commit_modeset_enables(dev, state);
> +
> + drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> + drm_atomic_helper_cleanup_planes(dev, state);
> + drm_atomic_state_free(state);
> +}
> +
> +static void hdlcd_atomic_work(struct work_struct *work)
> +{
> + struct hdlcd_drm_private *hdlcd;
> +
> + hdlcd = container_of(work, struct hdlcd_drm_private, work);
> + hdlcd_atomic_complete(hdlcd->crtc.dev, hdlcd->state);
> +}
> +
> static int hdlcd_atomic_commit(struct drm_device *dev,
> struct drm_atomic_state *state, bool nonblock)
> {
> - return drm_atomic_helper_commit(dev, state, false);
> + struct hdlcd_drm_private *hdlcd = dev->dev_private;
> + int err = drm_atomic_helper_prepare_planes(dev, state);
> +
> + if (err)
> + return err;
> +
> + if (nonblock && !list_empty(&hdlcd->work.entry)) {
> + /* pending commit found, bail out */
> + return -EBUSY;
> + }
> +
> + mutex_lock(&hdlcd->work_lock);
> + flush_work(&hdlcd->work);
> +
> + /*
> + * This is the point of non return, where software commits to
> + * handle the new state
> + */
> + drm_atomic_helper_swap_state(dev, state);
> + hdlcd->state = state;
> +
> + if (nonblock)
> + schedule_work(&hdlcd->work);
> + else
> + hdlcd_atomic_complete(dev, state);
> +
> + mutex_unlock(&hdlcd->work_lock);
> +
> + return 0;
> }
>
> static const struct drm_mode_config_funcs hdlcd_mode_config_funcs = {
> @@ -160,24 +213,9 @@ static irqreturn_t hdlcd_irq(int irq, void *arg)
> atomic_inc(&hdlcd->vsync_count);
>
> #endif
> - if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> - bool events_sent = false;
> - unsigned long flags;
> - struct drm_pending_vblank_event *e, *t;
> -
> + if (irq_status & HDLCD_INTERRUPT_VSYNC)
> drm_crtc_handle_vblank(&hdlcd->crtc);
>
> - spin_lock_irqsave(&drm->event_lock, flags);
> - list_for_each_entry_safe(e, t, &hdlcd->event_list, base.link) {
> - list_del(&e->base.link);
> - drm_crtc_send_vblank_event(&hdlcd->crtc, e);
> - events_sent = true;
> - }
> - if (events_sent)
> - drm_crtc_vblank_put(&hdlcd->crtc);
> - spin_unlock_irqrestore(&drm->event_lock, flags);
> - }
> -
> /* acknowledge interrupt(s) */
> hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, irq_status);
>
> @@ -200,9 +238,13 @@ static int hdlcd_irq_postinstall(struct drm_device *drm)
>
> /* enable debug interrupts */
> irq_mask |= HDLCD_DEBUG_INT_MASK;
> +#endif
> +
> + /* enable vsync interrupts */
> + irq_mask |= HDLCD_INTERRUPT_VSYNC;
>
> hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
> -#endif
> +
> return 0;
> }
>
> @@ -225,20 +267,11 @@ static void hdlcd_irq_uninstall(struct drm_device *drm)
>
> static int hdlcd_enable_vblank(struct drm_device *drm, unsigned int crtc)
> {
> - struct hdlcd_drm_private *hdlcd = drm->dev_private;
> - unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> -
> - hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
> -
> return 0;
> }
>
> static void hdlcd_disable_vblank(struct drm_device *drm, unsigned int crtc)
> {
> - struct hdlcd_drm_private *hdlcd = drm->dev_private;
> - unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> -
> - hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
> }
>
> #ifdef CONFIG_DEBUG_FS
> @@ -271,6 +304,7 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
> static struct drm_info_list hdlcd_debugfs_list[] = {
> { "interrupt_count", hdlcd_show_underrun_count, 0 },
> { "clocks", hdlcd_show_pxlclock, 0 },
> + { "fb", drm_fb_cma_debugfs_show, 0 },
> };
>
> static int hdlcd_debugfs_init(struct drm_minor *minor)
> @@ -354,6 +388,8 @@ static int hdlcd_drm_bind(struct device *dev)
>
> drm->dev_private = hdlcd;
> dev_set_drvdata(dev, drm);
> + mutex_init(&hdlcd->work_lock);
> + INIT_WORK(&hdlcd->work, hdlcd_atomic_work);
>
> hdlcd_setup_mode_config(drm);
> ret = hdlcd_load(drm, 0);
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
> index e7cea82..92822e9 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.h
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.h
> @@ -9,11 +9,12 @@ struct hdlcd_drm_private {
> void __iomem *mmio;
> struct clk *clk;
> struct drm_fbdev_cma *fbdev;
> - struct drm_framebuffer *fb;
> struct list_head event_list;
> struct drm_crtc crtc;
> struct drm_plane *plane;
> struct drm_atomic_state *state;
> + struct work_struct work;
> + struct mutex work_lock;
> #ifdef CONFIG_DEBUG_FS
> atomic_t buffer_underrun_count;
> atomic_t bus_error_count;
> --
> 2.8.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list