Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 16, 2020 at 05:22:34PM +0000, Emil Velikov wrote:
> Hi all,
> 
> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index 7cf3cf936547..23d2f51fc1d4 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > >       /* Self refresh should be canceled when a new update is available */
> > >       state->active = drm_atomic_crtc_effectively_active(state);
> > >       state->self_refresh_active = false;
> > > +
> > > +     if (drm_dev_has_vblank(crtc->dev))
> > > +             state->no_vblank = true;
> > > +     else
> > > +             state->no_vblank = false;
> > >  }
> > >  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
> >
> > I think the if/else branches are in the wrong order.

Yeah fumbled that.

> > But generally speaking, is it really that easy? The xen driver already
> > has to work around simple-kms's auto-enabling of no_vblank (see patch
> > 4). Maybe this settings interferes with other drivers as well. At least
> > the calls for sending fake vblanks should be removed from all affected
> > drivers.

Hm xen is really special, in that it has a flip complete event, but not a
vblank. I think forcing drivers to overwrite stuff in that case makes
sense.

> I'm not sure if setting no_vblank based on dev->num_crtcs is the correct thing.
> From the original commit and associated description for no_vblank:
> 
> In some cases CRTCs are active but are not able to generating events, at
> least not at every frame at it's expected to.
> This is typically the case when the CRTC is feeding a writeback connector...

Yeah, but Thomas' series here wants to extend that. And I think if we roll
this out the common case will be "no hw vblank", and the writeback special
case is going to be the exception to the exception. Yup, patch 1 that
updates the docs doesn't reflect that, which is why I'm bringing up more
suggestions here around code & semantics of all these pieces to make them
do the most reasonable thing for most of the drivers.

> Reflects the ability of a CRTC to send VBLANK events....
> 
> 
> The proposed handling of no_vblank feels a little dirty, although
> nothing better comes to mind.
> Nevertheless code seems perfectly reasonable, so if it were me I'd merge it.

The idea with setting it very early is that drivers can overwrite it very
easily. Feels slightly dirty, so I guess we could also set it somewhere in
the atomic_helper_check function (similar to how we set the various
crtc->*_changed flags, but we're not entirely consistent on these either).

For the overall thing what feels irky to me is making this no_vblank
default logic (however we end up computing it in the end, whether like
this or what I suggested) specific to simple pipe helpers feels kinda
wrong. Simple pipe tends to have a higher ratio of drivers for hw without
vblank support, but by far not the only ones. Having that special case
feels confusing to me (and likely will trip up some people, vblank and
event handling is already a huge source of confusion in drm).

One idea behind drm_dev_has_vblank() is also that we could formalize a bit
all that, at least for the usual case - xen and maybe others being some
exceptions as usual (hence definitely not something the core code should
handle).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel