| Message ID | 864lupd1cv.fsf@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 06, 2017 at 08:36:00AM -0700, Keith Packard wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > > > A few nits below, but looks good otherwise. > > Thanks. > > >> 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 *. > > As 'dev' isn't used anyways, this seems like a fine plan. > > >> + 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. > > Yes, this prepares for the new events to make that patch smaller. The > places where the data are still unconditionally assigned should know > that the event in the struct is either a VBLANK or FLIP_COMPLETE. Yeah, I realized that after reading the next patch carefully. > >> + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > > > > This'll oops on ums drivers since kms isn't set up. > > How about this fix? > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 857b7cf011e1..e39b2bd074e4 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -1355,7 +1355,6 @@ 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); > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > struct drm_pending_vblank_event *e; > struct timespec now; > @@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe, > e->event.base.type = DRM_EVENT_VBLANK; > 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; > + e->event.vbl.crtc_id = 0; > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > + if (crtc) > + e->event.vbl.crtc_id = crtc->base.id; > + } > > spin_lock_irqsave(&dev->event_lock, flags); lgtm. > > Or maybe I shouldn't have told you this and seized this opportunity to > > break all the old drivers :-) > > You now know my evil plan :-) :-) -Daniel
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 857b7cf011e1..e39b2bd074e4 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1355,7 +1355,6 @@ 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); struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; struct drm_pending_vblank_event *e; struct timespec now; @@ -1373,7 +1372,12 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe, e->event.base.type = DRM_EVENT_VBLANK; 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; + e->event.vbl.crtc_id = 0; + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); + if (crtc) + e->event.vbl.crtc_id = crtc->base.id; + } spin_lock_irqsave(&dev->event_lock, flags);