Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755308AbcC1TS1 (ORCPT ); Mon, 28 Mar 2016 15:18:27 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:42883 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751164AbcC1TSP (ORCPT ); Mon, 28 Mar 2016 15:18:15 -0400 From: Laurent Pinchart To: Sebastian Reichel Cc: Tony Lindgren , Aaro Koskinen , Tomi Valkeinen , David Airlie , linux-omap@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 07/23] drm: omapdrm: crtc: switch pending variable to atomic bitset Date: Sat, 26 Mar 2016 18:30:02 +0200 Message-ID: <10595698.j3sjVXcOKr@avalon> User-Agent: KMail/4.14.10 (Linux/4.1.15-gentoo-r1; KDE/4.14.16; x86_64; ; ) In-Reply-To: <1457455195-1938-8-git-send-email-sre@kernel.org> References: <1457455195-1938-1-git-send-email-sre@kernel.org> <1457455195-1938-8-git-send-email-sre@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4809 Lines: 163 Hi Sebastian, Thank you for the patch. On Tuesday 08 Mar 2016 17:39:39 Sebastian Reichel wrote: > Having the pending variable available as atomic bit helps > with the later addition of manually updated display support. > > Signed-off-by: Sebastian Reichel > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2ed0754ed19e..5ef27664bcfa > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -28,6 +28,11 @@ > > #define to_omap_crtc(x) container_of(x, struct omap_crtc, base) > > +enum omap_crtc_state { I find that using an enum and calling it omap_crtc_state is confusing, it seems to imply that the CRTC state will be one of the enumerated values, while the values are actually non-exclusive bits in a bitmask. > + crtc_enabled = 0, You don't seem to be using this bit in this patch, you can define it in patch 08/23. > + crtc_pending = 1 Please name this OMAP_CRTC_STATE_PENDING. > +}; Please add a short description of each bit in a comment above the enum. > + > struct omap_crtc { > struct drm_crtc base; > > @@ -49,7 +54,7 @@ struct omap_crtc { > > bool ignore_digit_sync_lost; > > - bool pending; > + unsigned long state; > wait_queue_head_t pending_wait; > }; > > @@ -81,7 +86,7 @@ int omap_crtc_wait_pending(struct drm_crtc *crtc) > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > > return wait_event_timeout(omap_crtc->pending_wait, > - !omap_crtc->pending, > + !test_bit(crtc_pending, &omap_crtc->state), > msecs_to_jiffies(50)); > } > > @@ -311,10 +316,8 @@ static void omap_crtc_vblank_irq(struct omap_drm_irq > *irq, uint32_t irqstatus) > > __omap_irq_unregister(dev, &omap_crtc->vblank_irq); > > - rmb(); > - WARN_ON(!omap_crtc->pending); > - omap_crtc->pending = false; > - wmb(); > + if (!test_and_clear_bit(crtc_pending, &omap_crtc->state)) > + dev_warn(dev->dev, "pending bit was not set in vblank irq"); Documentation/atomic_ops.txt states that the atomic bit ops must be atomic and include memory barriers, but I'm confused by the ARM implementation. The constant bit number version ____atomic_test_and_clear_bit() uses raw_local_irq_save() and raw_low_irq_restore() for synchronization, which expand to arch_local_irq_save() and arch_local_irq_restore(). On ARMv6 and newer, the functions are defined as static inline unsigned long arch_local_irq_save(void) { unsigned long flags; asm volatile( " mrs %0, " IRQMASK_REG_NAME_R " @ arch_local_irq_save\n" " cpsid i" : "=r" (flags) : : "memory", "cc"); return flags; } and static inline void arch_local_irq_restore(unsigned long flags) { asm volatile( " msr " IRQMASK_REG_NAME_W ", %0 @ local_irq_restore" : : "r" (flags) : "memory", "cc"); } I'm probably missing something obvious, but I don't see the memory barriers :-/ > /* wake up userspace */ > omap_crtc_complete_page_flip(&omap_crtc->base); > @@ -351,13 +354,12 @@ static bool omap_crtc_mode_fixup(struct drm_crtc > *crtc, static void omap_crtc_enable(struct drm_crtc *crtc) > { > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > + struct drm_device *dev = crtc->dev; > > DBG("%s", omap_crtc->name); > > - rmb(); > - WARN_ON(omap_crtc->pending); > - omap_crtc->pending = true; > - wmb(); > + if (test_and_set_bit(crtc_pending, &omap_crtc->state)) > + dev_warn(dev->dev, "crtc enable while pending bit set!"); > > omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); > > @@ -397,6 +399,7 @@ static void omap_crtc_atomic_flush(struct drm_crtc > *crtc, struct drm_crtc_state *old_crtc_state) { > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > + struct drm_device *dev = crtc->dev; > > WARN_ON(omap_crtc->vblank_irq.registered); > > @@ -404,10 +407,8 @@ static void omap_crtc_atomic_flush(struct drm_crtc > *crtc, > > DBG("%s: GO", omap_crtc->name); > > - rmb(); > - WARN_ON(omap_crtc->pending); > - omap_crtc->pending = true; > - wmb(); > + if (test_and_set_bit(crtc_pending, &omap_crtc->state)) > + dev_warn(dev->dev, "atomic flush while pending bit set!"); > > dispc_mgr_go(omap_crtc->channel); > omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); > @@ -509,6 +510,8 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, > > init_waitqueue_head(&omap_crtc->pending_wait); > > + omap_crtc->state = 0; > + > omap_crtc->channel = channel; > omap_crtc->name = channel_names[channel]; -- Regards, Laurent Pinchart