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
