On Thu, Jul 06, 2017 at 08:36:00AM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@xxxxxxxx> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
