Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753071AbdGFHaz (ORCPT ); Thu, 6 Jul 2017 03:30:55 -0400 Received: from mail-lf0-f67.google.com ([209.85.215.67]:34932 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752666AbdGFHay (ORCPT ); Thu, 6 Jul 2017 03:30:54 -0400 Date: Thu, 6 Jul 2017 09:30:49 +0200 From: Daniel Vetter To: Keith Packard Cc: linux-kernel@vger.kernel.org, Dave Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org Subject: Re: [PATCH 2/3] drm: Reorganize drm_pending_event to support future event types Message-ID: <20170706073049.mfmjvujx4szf6qji@phenom.ffwll.local> Mail-Followup-To: Keith Packard , linux-kernel@vger.kernel.org, Dave Airlie , dri-devel@lists.freedesktop.org References: <20170705221013.27940-1-keithp@keithp.com> <20170705221013.27940-3-keithp@keithp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170705221013.27940-3-keithp@keithp.com> X-Operating-System: Linux phenom 4.11.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8549 Lines: 228 On Wed, Jul 05, 2017 at 03:10:12PM -0700, Keith Packard wrote: > Place drm_event_vblank in a new union that includes that and a bare > drm_event structure. This will allow new members of that union to be > added in the future without changing code related to the existing vbl > event type. > > Assignments to the crtc_id field are now done when the event is > allocated, rather than when delievered. This way, delivery doesn't > need to have the crtc ID available. > > Signed-off-by: Keith Packard A few nits below, but looks good otherwise. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 7 ++++--- > drivers/gpu/drm/drm_plane.c | 2 +- > drivers/gpu/drm/drm_vblank.c | 27 ++++++++++++++++----------- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 4 ++-- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 ++-- > include/drm/drm_vblank.h | 8 +++++++- > 6 files changed, 32 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index c0f336d23f9c..f569d7f03f3c 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1839,7 +1839,7 @@ int drm_atomic_debugfs_init(struct drm_minor *minor) > */ > > static struct drm_pending_vblank_event *create_vblank_event( > - struct drm_device *dev, uint64_t user_data) > + struct drm_device *dev, struct drm_crtc *crtc, uint64_t user_data) Nit: Please also drop the dev argument, we have crtc->dev easily available. That fits better into my long-term goal of getting rid of the (dev, pipe) pairs everywhere in the vblank code and fully switching over to drm_crtc *. > { > struct drm_pending_vblank_event *e = NULL; > > @@ -1849,7 +1849,8 @@ static struct drm_pending_vblank_event *create_vblank_event( > > e->event.base.type = DRM_EVENT_FLIP_COMPLETE; > e->event.base.length = sizeof(e->event); > - e->event.user_data = user_data; > + e->event.vbl.crtc_id = crtc->base.id; > + e->event.vbl.user_data = user_data; > > return e; > } > @@ -2052,7 +2053,7 @@ static int prepare_crtc_signaling(struct drm_device *dev, > if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) { > struct drm_pending_vblank_event *e; > > - e = create_vblank_event(dev, arg->user_data); > + e = create_vblank_event(dev, crtc, arg->user_data); > if (!e) > return -ENOMEM; > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 5dc8c4350602..fe9f31285bc2 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -918,7 +918,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, > } > e->event.base.type = DRM_EVENT_FLIP_COMPLETE; > e->event.base.length = sizeof(e->event); > - e->event.user_data = page_flip->user_data; > + e->event.vbl.user_data = page_flip->user_data; > ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base); > if (ret) { > kfree(e); > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index f55f997c0b8f..9ae170857ef6 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -807,13 +807,18 @@ static void send_vblank_event(struct drm_device *dev, > struct drm_pending_vblank_event *e, > u64 seq, struct timespec *now) > { > - e->event.sequence = seq; > - e->event.tv_sec = now->tv_sec; > - e->event.tv_usec = now->tv_nsec / 1000; > - > - trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, > - e->event.sequence); > - > + switch (e->event.base.type) { > + case DRM_EVENT_VBLANK: > + case DRM_EVENT_FLIP_COMPLETE: > + if (seq) > + e->event.vbl.sequence = (u32) seq; > + if (now) { > + e->event.vbl.tv_sec = now->tv_sec; > + e->event.vbl.tv_usec = now->tv_nsec / 1000; > + } > + break; > + } Not sure why this change? Also prep for the new, presumably extended events? Seems at least slightly inconsistent with other paths, where we still unconditionally fill it in. > + trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq); > drm_send_event_locked(dev, &e->base); > } > > @@ -865,7 +870,6 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, > > e->pipe = pipe; > e->sequence = drm_vblank_count(dev, pipe); > - e->event.crtc_id = crtc->base.id; > list_add_tail(&e->base.link, &dev->vblank_event_list); > } > EXPORT_SYMBOL(drm_crtc_arm_vblank_event); > @@ -897,7 +901,6 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc, > now = get_drm_timestamp(); > } > e->pipe = pipe; > - e->event.crtc_id = crtc->base.id; > send_vblank_event(dev, e, seq, &now); > } > EXPORT_SYMBOL(drm_crtc_send_vblank_event); > @@ -1342,6 +1345,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe, > union drm_wait_vblank *vblwait, > struct drm_file *file_priv) > { > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); This'll oops on ums drivers since kms isn't set up. And we can call this from ums drivers in the old vblank ioctl. My long-term goal is to completely separate these two worlds, and exclusive deal with drm_crtc * in the kms side (and only compute the pipe index when needed). But we're not there yet. I also want to split it into two files (drm_crtc_vblank.c and drm_legacy_vblank.c) and make sure we consistently use drm_crtc_vblank_ as the new-world prefix. Probably the simplest option is to extend this to take all three (dev, pipe, crtc) as arguments, and then pass NULL for the old vblank ioctl, and the right pointer for the new stuff. Or you stuff a DRIVER_MODESET check at the right spot somewhere. Or maybe I shouldn't have told you this and seized this opportunity to break all the old drivers :-) > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > struct drm_pending_vblank_event *e; > struct timespec now; > @@ -1357,8 +1361,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe, > > e->pipe = pipe; > e->event.base.type = DRM_EVENT_VBLANK; > - e->event.base.length = sizeof(e->event); > - e->event.user_data = vblwait->request.signal; > + e->event.base.length = sizeof(e->event.vbl); > + e->event.vbl.user_data = vblwait->request.signal; > + e->event.vbl.crtc_id = crtc ? crtc->base.id : 0; > > spin_lock_irqsave(&dev->event_lock, flags); > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > index 8d7dc9def7c2..c13b97338310 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > @@ -358,8 +358,8 @@ static int vmw_sou_crtc_page_flip(struct drm_crtc *crtc, > > ret = vmw_event_fence_action_queue(file_priv, fence, > &event->base, > - &event->event.tv_sec, > - &event->event.tv_usec, > + &event->event.vbl.tv_sec, > + &event->event.vbl.tv_usec, > true); > } > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > index bad31bdf09b6..4e329588ce9c 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > @@ -544,8 +544,8 @@ static int vmw_stdu_crtc_page_flip(struct drm_crtc *crtc, > > ret = vmw_event_fence_action_queue(file_priv, fence, > &event->base, > - &event->event.tv_sec, > - &event->event.tv_usec, > + &event->event.vbl.tv_sec, > + &event->event.vbl.tv_usec, > true); > vmw_fence_obj_unreference(&fence); > } else { > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h > index 68e99177fff3..3ef7ddc5db5f 100644 > --- a/include/drm/drm_vblank.h > +++ b/include/drm/drm_vblank.h > @@ -54,7 +54,10 @@ struct drm_pending_vblank_event { > /** > * @event: Actual event which will be sent to userspace. > */ > - struct drm_event_vblank event; > + union { > + struct drm_event base; > + struct drm_event_vblank vbl; > + } event; > }; > > /** > @@ -163,6 +166,9 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc, > struct drm_pending_vblank_event *e); > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, > struct drm_pending_vblank_event *e); > +void drm_vblank_set_event(struct drm_pending_vblank_event *e, > + u64 *seq, > + struct timespec *now); > bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe); > bool drm_crtc_handle_vblank(struct drm_crtc *crtc); > int drm_crtc_vblank_get(struct drm_crtc *crtc); > -- > 2.11.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch