Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
On Fri, Jan 17, 2020 at 08:17:10AM +0100, Thomas Zimmermann wrote: > Hi > > Am 17.01.20 um 00:59 schrieb Daniel Vetter: > > On Thu, Jan 16, 2020 at 05:22:34PM +, Emil Velikov wrote: > >> Hi all, > >> > >> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann > >> 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 > > Default values should usually be 0 for zalloc and static initializers. > Should we rename no_vblank to has_vblank then? Hm, imo feels like hw without vblank is still the uncommon case. I'd leave this as-is, but also no objections if you feel like repainting :-) > > 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). > > Making it a default for simple KMS was only the start. I intended to > cover all drivers at some point. I just didn't want to go through all > drivers at once. > > I guess for the patchset's v3 I'll audit all drivers for the use of > no_blank and drm_crtc_send_vblank_event(); and convert the possible > candidates. Yeah it's a pain, thanks for volunteering. Just figured the half-step here is too much in the uncanney valley. If we're going to polish this, let's do it right (and we have plenty enough drivers to make sure what we pick will be a solid choice I think). -Daniel > > Best regards > Thomas > > > > > 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 > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imend
Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
Hi Am 22.01.20 um 09:11 schrieb Daniel Vetter: > On Fri, Jan 17, 2020 at 08:17:10AM +0100, Thomas Zimmermann wrote: >> Hi >> >> Am 17.01.20 um 00:59 schrieb Daniel Vetter: >>> On Thu, Jan 16, 2020 at 05:22:34PM +, Emil Velikov wrote: Hi all, On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann 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 >> >> Default values should usually be 0 for zalloc and static initializers. >> Should we rename no_vblank to has_vblank then? > > Hm, imo feels like hw without vblank is still the uncommon case. I'd leave > this as-is, but also no objections if you feel like repainting :-) Not a bit. ;) > >>> 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). >> >> Making it a default for simple KMS was only the start. I intended to >> cover all drivers at some point. I just didn't want to go through all >> drivers at once. >> >> I guess for the patchset's v3 I'll audit all drivers for the use of >> no_blank and drm_crtc_send_vblank_event(); and convert the possible >> candidates. > > Yeah it's a pain, thanks for volunteering. Just figured the half-step here > is too much in the uncanney valley. If we're going to polish this, let's > do it right (and we have plenty enough drivers to make sure what we pick > will be a solid choice I think). I went through the drivers and updated them. It's the ones covered here plus some more virtual HW. My search heuristic was to look for drivers that call drm_crtc_send_vblank_event() but do not call drm_vblank_init(). Please see v3 of this patchset. It should be pending on this ML. Best regards Thomas > -Daniel > >> >> Best regards >> Thomas >> >>> >>> One idea behind drm_dev_has_vblank() is also that we could formalize
Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
Hi Am 17.01.20 um 00:59 schrieb Daniel Vetter: > On Thu, Jan 16, 2020 at 05:22:34PM +, Emil Velikov wrote: >> Hi all, >> >> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann 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 Default values should usually be 0 for zalloc and static initializers. Should we rename no_vblank to has_vblank then? > 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). Making it a default for simple KMS was only the start. I intended to cover all drivers at some point. I just didn't want to go through all drivers at once. I guess for the patchset's v3 I'll audit all drivers for the use of no_blank and drm_crtc_send_vblank_event(); and convert the possible candidates. Best regards Thomas > > 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 > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
On Thu, Jan 16, 2020 at 05:22:34PM +, Emil Velikov wrote: > Hi all, > > On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
Hi all, On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann 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. > > 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. > 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... 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. HTH Emil ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
Hi Am 16.01.20 um 07:41 schrieb Daniel Vetter: > On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote: >> In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events >> if struct drm_crtc_state.no_vblank is enabled in the check() callbacks. >> >> For drivers that have neither an enable_vblank() callback nor a check() >> callback, the simple-KMS helpers enable VBLANK generation by default. This >> simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI >> DPI helpers. The driver for Xen explicitly disables no_vblank, as it has >> its own logic for sending these events. >> >> Signed-off-by: Thomas Zimmermann > >> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c >> b/drivers/gpu/drm/drm_simple_kms_helper.c >> index 15fb516ae2d8..4414c7a5b2ce 100644 >> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >> @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct >> drm_plane *plane, >> if (!plane_state->visible) >> return 0; >> >> -if (!pipe->funcs || !pipe->funcs->check) >> -return 0; >> - >> -return pipe->funcs->check(pipe, plane_state, crtc_state); >> +if (pipe->funcs) { >> +if (pipe->funcs->check) >> +return pipe->funcs->check(pipe, plane_state, >> + crtc_state); >> +if (pipe->funcs->enable_vblank) >> +return 0; >> +} >> + >> +/* Drivers without VBLANK support have to fake VBLANK events. As >> + * there's no check() callback to enable this, set the no_vblank >> + * field by default. >> + */ > > The ->check callback is right above this comment ... I'm confused. I guess that comment isn't overly precise. What it means is that no_vblank would have to be set in check(), but the driver did not specify a check() function. So it has neither vblank support nor any way of setting no_vblank. Hence, the simple-kms helper sets no_vblank automatically. Maybe something to update for the patchset's v2. > >> +crtc_state->no_vblank = true; > > That's kinda not what I meant with handling this automatically. Instead > something like this: > > > 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. 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. Best regards Thomas > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 1659b13b178c..32cab3d3c872 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -81,6 +81,12 @@ > */ > #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100 > > +/* FIXME roll this out here in this file */ > +bool drm_dev_has_vblank(dev) > +{ > + return dev->num_crtcs; > +} > + > static bool > drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe, > ktime_t *tvblank, bool in_vblank_irq); > > > But maybe move the default value to some other/better place in the atomic > helpers, not sure what the best one is. > > Plus then in the documentation patch also highlight the link between > crtc_state->no_vblank and drm_dev_has_vblank respectively > drm_device.num_crtcs. > > That should plug this issue once for all across the board. > > There's still the fun between having the vblank callbacks and the > drm_vblank setup, but that's a much older can of worms ... > -Daniel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote: > In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events > if struct drm_crtc_state.no_vblank is enabled in the check() callbacks. > > For drivers that have neither an enable_vblank() callback nor a check() > callback, the simple-KMS helpers enable VBLANK generation by default. This > simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI > DPI helpers. The driver for Xen explicitly disables no_vblank, as it has > its own logic for sending these events. > > Signed-off-by: Thomas Zimmermann > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c > b/drivers/gpu/drm/drm_simple_kms_helper.c > index 15fb516ae2d8..4414c7a5b2ce 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct > drm_plane *plane, > if (!plane_state->visible) > return 0; > > - if (!pipe->funcs || !pipe->funcs->check) > - return 0; > - > - return pipe->funcs->check(pipe, plane_state, crtc_state); > + if (pipe->funcs) { > + if (pipe->funcs->check) > + return pipe->funcs->check(pipe, plane_state, > + crtc_state); > + if (pipe->funcs->enable_vblank) > + return 0; > + } > + > + /* Drivers without VBLANK support have to fake VBLANK events. As > + * there's no check() callback to enable this, set the no_vblank > + * field by default. > + */ The ->check callback is right above this comment ... I'm confused. > + crtc_state->no_vblank = true; That's kinda not what I meant with handling this automatically. Instead something like this: 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); diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 1659b13b178c..32cab3d3c872 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -81,6 +81,12 @@ */ #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100 +/* FIXME roll this out here in this file */ +bool drm_dev_has_vblank(dev) +{ + return dev->num_crtcs; +} + static bool drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe, ktime_t *tvblank, bool in_vblank_irq); But maybe move the default value to some other/better place in the atomic helpers, not sure what the best one is. Plus then in the documentation patch also highlight the link between crtc_state->no_vblank and drm_dev_has_vblank respectively drm_device.num_crtcs. That should plug this issue once for all across the board. There's still the fun between having the vblank callbacks and the drm_vblank setup, but that's a much older can of worms ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization