[PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
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 that has no job queued. In this situation the CRTC is usually stopped until a new job is queued, and this can lead to timeouts when part of the pipeline is updated but no new jobs are queued to the active writeback connector. In order to solve that, we add a ->no_vblank flag to drm_crtc_state and ask the CRTC drivers to set it to true when they know they're not able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() helper can then be used to fake VBLANKs at commit time. Signed-off-by: Boris Brezillon --- drivers/gpu/drm/drm_atomic_helper.c | 40 + include/drm/drm_atomic_helper.h | 1 + include/drm/drm_crtc.h | 15 ++ 3 files changed, 56 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 69063bcf2334..ca586993c2a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); +/** + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed + * @old_state: atomic state object with old state structures + * + * This function walks all CRTCs and fake VBLANK events on those with + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL. + * The primary use of this function is writeback connectors working in oneshot + * mode and faking VBLANK events. In this case they only fake the VBLANK event + * when a job is queued, and any change to the pipeline that does not touch the + * connector is leading to timeouts when calling + * drm_atomic_helper_wait_for_vblanks() or + * drm_atomic_helper_wait_for_flip_done(). + * + * This is part of the atomic helper support for nonblocking commits, see + * drm_atomic_helper_setup_commit() for an overview. + */ +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) +{ + struct drm_crtc_state *old_crtc_state, *new_crtc_state; + struct drm_crtc *crtc; + int i; + + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, + new_crtc_state, i) { + unsigned long flags; + + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) + continue; + + spin_lock_irqsave(&old_state->dev->event_lock, flags); + if (new_crtc_state->event) { + drm_crtc_send_vblank_event(crtc, + new_crtc_state->event); + new_crtc_state->event = NULL; + } + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); + } +} +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); + /** * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit * @old_state: atomic state object with old state structures diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 26aaba58d6ce..99e2a5297c69 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct drm_atomic_state *state, int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock); void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 23eddbccab10..7435dc66c08a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs; * @zpos_changed: zpos values of planes on this crtc have been updated * @color_mgmt_changed: color management properties have changed (degamma or * gamma LUT or CSC matrix) + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This state + * usually depends on the pipeline configuration, and the main usuage is + * CRTCs feeding a writeback connector operating in oneshot mode. In this + * case the VBLANK event is only generated when a job is queued to the + * writeback connector, and we want the core to fake VBLANK events when + * this part of the pipeline hasn't changed but others had or when the + * CRTC and connectors are disabled. + * __drm_atomic_helper_crtc_duplicate_state() will the value from the + * current state, the CRTC driver is then responsible for updating this + * field when needed. + * Note that,
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote: > 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 > that has no job queued. In this situation the CRTC is usually stopped > until a new job is queued, and this can lead to timeouts when part of > the pipeline is updated but no new jobs are queued to the active > writeback connector. > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state > and ask the CRTC drivers to set it to true when they know they're not > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() > helper can then be used to fake VBLANKs at commit time. > > Signed-off-by: Boris Brezillon Reviewed-by: Liviu Dudau > --- > drivers/gpu/drm/drm_atomic_helper.c | 40 > + > include/drm/drm_atomic_helper.h | 1 + > include/drm/drm_crtc.h | 15 ++ > 3 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 69063bcf2334..ca586993c2a2 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct > drm_atomic_state *old_state) > } > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > +/** > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed > + * @old_state: atomic state object with old state structures > + * > + * This function walks all CRTCs and fake VBLANK events on those with > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL. > + * The primary use of this function is writeback connectors working in > oneshot > + * mode and faking VBLANK events. In this case they only fake the VBLANK > event > + * when a job is queued, and any change to the pipeline that does not touch > the > + * connector is leading to timeouts when calling > + * drm_atomic_helper_wait_for_vblanks() or > + * drm_atomic_helper_wait_for_flip_done(). > + * > + * This is part of the atomic helper support for nonblocking commits, see > + * drm_atomic_helper_setup_commit() for an overview. > + */ > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) > +{ > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > + struct drm_crtc *crtc; > + int i; > + > + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > + new_crtc_state, i) { > + unsigned long flags; > + > + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) > + continue; > + > + spin_lock_irqsave(&old_state->dev->event_lock, flags); > + if (new_crtc_state->event) { > + drm_crtc_send_vblank_event(crtc, > +new_crtc_state->event); > + new_crtc_state->event = NULL; > + } > + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); > + } > +} > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); > + > /** > * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit > * @old_state: atomic state object with old state structures > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 26aaba58d6ce..99e2a5297c69 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct > drm_atomic_state *state, > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > bool nonblock); > void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); > void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); > void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 23eddbccab10..7435dc66c08a 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs; > * @zpos_changed: zpos values of planes on this crtc have been updated > * @color_mgmt_changed: color management properties have changed (degamma or > * gamma LUT or CSC matrix) > + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This > state > + * usually depends on the pipeline configuration, and the main usuage is > + * CRTCs feeding a writeback connector operating in oneshot mode. In this > + * case the VBLANK event is only generated when a job is queued to the > + * writeback connector, and we want the core to fake VBLANK events when > + * this part of the pipeline hasn't changed but
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote: > 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 > that has no job queued. In this situation the CRTC is usually stopped > until a new job is queued, and this can lead to timeouts when part of > the pipeline is updated but no new jobs are queued to the active > writeback connector. > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state > and ask the CRTC drivers to set it to true when they know they're not > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() > helper can then be used to fake VBLANKs at commit time. > > Signed-off-by: Boris Brezillon > --- > drivers/gpu/drm/drm_atomic_helper.c | 40 > + > include/drm/drm_atomic_helper.h | 1 + > include/drm/drm_crtc.h | 15 ++ > 3 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 69063bcf2334..ca586993c2a2 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct > drm_atomic_state *old_state) > } > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > +/** > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed > + * @old_state: atomic state object with old state structures > + * > + * This function walks all CRTCs and fake VBLANK events on those with > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL. > + * The primary use of this function is writeback connectors working in > oneshot > + * mode and faking VBLANK events. In this case they only fake the VBLANK > event > + * when a job is queued, and any change to the pipeline that does not touch > the > + * connector is leading to timeouts when calling > + * drm_atomic_helper_wait_for_vblanks() or > + * drm_atomic_helper_wait_for_flip_done(). > + * > + * This is part of the atomic helper support for nonblocking commits, see > + * drm_atomic_helper_setup_commit() for an overview. > + */ > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) > +{ > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > + struct drm_crtc *crtc; > + int i; > + > + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > + new_crtc_state, i) { > + unsigned long flags; > + > + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) Uh, this essentially makes it impossible to reset no_vblank. For control flow state bits we only check the new state for it (see e.g. the various *_changed or plane_bits or whatever). > + continue; > + > + spin_lock_irqsave(&old_state->dev->event_lock, flags); > + if (new_crtc_state->event) { > + drm_crtc_send_vblank_event(crtc, > +new_crtc_state->event); > + new_crtc_state->event = NULL; > + } > + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); > + } > +} > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); > + > /** > * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit > * @old_state: atomic state object with old state structures > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 26aaba58d6ce..99e2a5297c69 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct > drm_atomic_state *state, > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > bool nonblock); > void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state); > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); > void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); > void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 23eddbccab10..7435dc66c08a 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -87,6 +87,20 @@ struct drm_plane_helper_funcs; > * @zpos_changed: zpos values of planes on this crtc have been updated > * @color_mgmt_changed: color management properties have changed (degamma or > * gamma LUT or CSC matrix) > + * @no_vblank: reflects the ability of a CRTC to send VBLANK events. This > state > + * usually depends on the pipeline configuration, and the main usuage is > + * CRTCs feeding a writeback connector operating in oneshot mode. In this > + * case the VBLANK event is only generated
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
On Mon, 2 Jul 2018 10:02:52 +0200 Daniel Vetter wrote: > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote: > > 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 > > that has no job queued. In this situation the CRTC is usually stopped > > until a new job is queued, and this can lead to timeouts when part of > > the pipeline is updated but no new jobs are queued to the active > > writeback connector. > > > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state > > and ask the CRTC drivers to set it to true when they know they're not > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() > > helper can then be used to fake VBLANKs at commit time. > > > > Signed-off-by: Boris Brezillon > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 40 > > + > > include/drm/drm_atomic_helper.h | 1 + > > include/drm/drm_crtc.h | 15 ++ > > 3 files changed, 56 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index 69063bcf2334..ca586993c2a2 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2051,6 +2051,46 @@ void drm_atomic_helper_wait_for_dependencies(struct > > drm_atomic_state *old_state) > > } > > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > > > +/** > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed > > + * @old_state: atomic state object with old state structures > > + * > > + * This function walks all CRTCs and fake VBLANK events on those with > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != NULL. > > + * The primary use of this function is writeback connectors working in > > oneshot > > + * mode and faking VBLANK events. In this case they only fake the VBLANK > > event > > + * when a job is queued, and any change to the pipeline that does not > > touch the > > + * connector is leading to timeouts when calling > > + * drm_atomic_helper_wait_for_vblanks() or > > + * drm_atomic_helper_wait_for_flip_done(). > > + * > > + * This is part of the atomic helper support for nonblocking commits, see > > + * drm_atomic_helper_setup_commit() for an overview. > > + */ > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) > > +{ > > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > > + struct drm_crtc *crtc; > > + int i; > > + > > + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > > + new_crtc_state, i) { > > + unsigned long flags; > > + > > + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) > > Uh, this essentially makes it impossible to reset no_vblank. I don't want ->no_vblank to be reset by the core. It's up to the CRTC driver to clear/set it when something changes in the pipeline. > For control > flow state bits we only check the new state for it (see e.g. the various > *_changed or plane_bits or whatever). I tried with !new_crtc_state->no_vblank only, but then it does not handle the case where the CRTC and connector are being disabled, and I end up with a timeout. > > > + continue; > > + > > + spin_lock_irqsave(&old_state->dev->event_lock, flags); > > + if (new_crtc_state->event) { > > + drm_crtc_send_vblank_event(crtc, > > + new_crtc_state->event); > > + new_crtc_state->event = NULL; > > + } > > + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); > > + } > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); > > + > > /** > > * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit > > * @old_state: atomic state object with old state structures > > diff --git a/include/drm/drm_atomic_helper.h > > b/include/drm/drm_atomic_helper.h > > index 26aaba58d6ce..99e2a5297c69 100644 > > --- a/include/drm/drm_atomic_helper.h > > +++ b/include/drm/drm_atomic_helper.h > > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct > > drm_atomic_state *state, > > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > >bool nonblock); > > void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state > > *state); > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); > > void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state); > > void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state); > > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index 23eddbccab10..7435dc66c08a 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_cr
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote: > On Mon, 2 Jul 2018 10:02:52 +0200 > Daniel Vetter wrote: > > > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote: > > > 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 > > > that has no job queued. In this situation the CRTC is usually stopped > > > until a new job is queued, and this can lead to timeouts when part of > > > the pipeline is updated but no new jobs are queued to the active > > > writeback connector. > > > > > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state > > > and ask the CRTC drivers to set it to true when they know they're not > > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() > > > helper can then be used to fake VBLANKs at commit time. > > > > > > Signed-off-by: Boris Brezillon > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 40 > > > + > > > include/drm/drm_atomic_helper.h | 1 + > > > include/drm/drm_crtc.h | 15 ++ > > > 3 files changed, 56 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > b/drivers/gpu/drm/drm_atomic_helper.c > > > index 69063bcf2334..ca586993c2a2 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -2051,6 +2051,46 @@ void > > > drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state > > > *old_state) > > > } > > > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > > > > > +/** > > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed > > > + * @old_state: atomic state object with old state structures > > > + * > > > + * This function walks all CRTCs and fake VBLANK events on those with > > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != > > > NULL. > > > + * The primary use of this function is writeback connectors working in > > > oneshot > > > + * mode and faking VBLANK events. In this case they only fake the VBLANK > > > event > > > + * when a job is queued, and any change to the pipeline that does not > > > touch the > > > + * connector is leading to timeouts when calling > > > + * drm_atomic_helper_wait_for_vblanks() or > > > + * drm_atomic_helper_wait_for_flip_done(). > > > + * > > > + * This is part of the atomic helper support for nonblocking commits, see > > > + * drm_atomic_helper_setup_commit() for an overview. > > > + */ > > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) > > > +{ > > > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > > > + struct drm_crtc *crtc; > > > + int i; > > > + > > > + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > > > + new_crtc_state, i) { > > > + unsigned long flags; > > > + > > > + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) > > > > Uh, this essentially makes it impossible to reset no_vblank. > > I don't want ->no_vblank to be reset by the core. It's up to the CRTC > driver to clear/set it when something changes in the pipeline. > > > For control > > flow state bits we only check the new state for it (see e.g. the various > > *_changed or plane_bits or whatever). > > I tried with !new_crtc_state->no_vblank only, but then it does not > handle the case where the CRTC and connector are being disabled, and I > end up with a timeout. Why that? You should have a new_crtc_state even when you disable the crtc, and you can set the ->no_vblank on that one too. With your current code it's essentially impossible to reset ->no_vblank, since it will only have effect for the _next_ atomic update, not the one you're doing right now. That doesn't make sense. E.g. try to do the same logic of checking both old&new state for e.g. needs_modeset and watch how all the logic totally falls to pieces. -Daniel > > > > > > + continue; > > > + > > > + spin_lock_irqsave(&old_state->dev->event_lock, flags); > > > + if (new_crtc_state->event) { > > > + drm_crtc_send_vblank_event(crtc, > > > +new_crtc_state->event); > > > + new_crtc_state->event = NULL; > > > + } > > > + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); > > > + } > > > +} > > > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); > > > + > > > /** > > > * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit > > > * @old_state: atomic state object with old state structures > > > diff --git a/include/drm/drm_atomic_helper.h > > > b/include/drm/drm_atomic_helper.h > > > index 26aaba58d6ce..99e2a5297c69 100644 > > > --- a/include/drm/drm_atomic_helper.h > > > +++ b/include/drm/drm_atomic_helper.h > > > @@
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
Forgot to address your other comments. On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote: > On Mon, 2 Jul 2018 10:02:52 +0200 > Daniel Vetter wrote: > > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote: > > > 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 > > > that has no job queued. In this situation the CRTC is usually stopped > > > until a new job is queued, and this can lead to timeouts when part of > > > the pipeline is updated but no new jobs are queued to the active > > > writeback connector. > > > > > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state > > > and ask the CRTC drivers to set it to true when they know they're not > > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() > > > helper can then be used to fake VBLANKs at commit time. > > > > > > Signed-off-by: Boris Brezillon > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 40 > > > + > > > include/drm/drm_atomic_helper.h | 1 + > > > include/drm/drm_crtc.h | 15 ++ > > > 3 files changed, 56 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > b/drivers/gpu/drm/drm_atomic_helper.c > > > index 69063bcf2334..ca586993c2a2 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -2051,6 +2051,46 @@ void > > > drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state > > > *old_state) > > > } > > > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > > > > > +/** > > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed > > > + * @old_state: atomic state object with old state structures > > > + * > > > + * This function walks all CRTCs and fake VBLANK events on those with > > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != > > > NULL. > > > + * The primary use of this function is writeback connectors working in > > > oneshot > > > + * mode and faking VBLANK events. In this case they only fake the VBLANK > > > event > > > + * when a job is queued, and any change to the pipeline that does not > > > touch the > > > + * connector is leading to timeouts when calling > > > + * drm_atomic_helper_wait_for_vblanks() or > > > + * drm_atomic_helper_wait_for_flip_done(). > > > + * > > > + * This is part of the atomic helper support for nonblocking commits, see > > > + * drm_atomic_helper_setup_commit() for an overview. > > > + */ > > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) > > > +{ > > > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > > > + struct drm_crtc *crtc; > > > + int i; > > > + > > > + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > > > + new_crtc_state, i) { > > > + unsigned long flags; > > > + > > > + if (!new_crtc_state->no_vblank && !old_crtc_state->no_vblank) > > > > Uh, this essentially makes it impossible to reset no_vblank. > > I don't want ->no_vblank to be reset by the core. It's up to the CRTC > driver to clear/set it when something changes in the pipeline. > > > For control > > flow state bits we only check the new state for it (see e.g. the various > > *_changed or plane_bits or whatever). > > I tried with !new_crtc_state->no_vblank only, but then it does not > handle the case where the CRTC and connector are being disabled, and I > end up with a timeout. > > > > > > + continue; > > > + > > > + spin_lock_irqsave(&old_state->dev->event_lock, flags); > > > + if (new_crtc_state->event) { > > > + drm_crtc_send_vblank_event(crtc, > > > +new_crtc_state->event); > > > + new_crtc_state->event = NULL; > > > + } > > > + spin_unlock_irqrestore(&old_state->dev->event_lock, flags); > > > + } > > > +} > > > +EXPORT_SYMBOL(drm_atomic_helper_fake_vblank); > > > + > > > /** > > > * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit > > > * @old_state: atomic state object with old state structures > > > diff --git a/include/drm/drm_atomic_helper.h > > > b/include/drm/drm_atomic_helper.h > > > index 26aaba58d6ce..99e2a5297c69 100644 > > > --- a/include/drm/drm_atomic_helper.h > > > +++ b/include/drm/drm_atomic_helper.h > > > @@ -100,6 +100,7 @@ int __must_check drm_atomic_helper_swap_state(struct > > > drm_atomic_state *state, > > > int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > > > bool nonblock); > > > void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state > > > *state); > > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *state); > > > void drm_atomic_helper_commi
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
On Mon, 2 Jul 2018 10:40:54 +0200 Daniel Vetter wrote: > > > > > > > + * Note that, even when no_blank is set to true, the CRTC driver > > > > can still > > > > + * steal the &drm_crtc_state.event object and send the event on > > > > its own. > > > > + * That's usually what happens when a job is queued to the > > > > writeback > > > > + * connector. > > > > > > The last sentence is confusing imo. Just drop it? > > > > Yes, I know, but it's also important to state that the ->no_blank + > > event == NULL is a valid combination, and just means that the driver > > decided to generate the event (that happens when a new WB job is > > queued). > > Then make it more explicit, as-is I had no idea what you meant exactly. > What about Your suggestion is missing :P. > > > > > > > > Please use the inline comment style for struct members, and then also > > > polish the formatting a bit (e.g. paragraph breaks, which are only > > > possible with the inline style). > > > > I considered that, but the other fields were already documented in the > > single block above the struct, so I thought keeping things consistent > > was better. Should I just add this field doc inline and keep the other > > ones where they are, or should I add a patch moving all docs inline? > > There's _lots_ of inline comments already, and all new ones should be > inline. The only reason I haven't done the conversion to all of them is > that it would be a nice opportunity to update/clean up the comments > (often there's a lot more to say than what's captured in the single line), > which is why it didn't happen yet. Just moving the comments without > updating them seems less useful imo. > > I'd just do the inline comment for this, that's what everyone else is > doing too. Okay, will do. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/8] drm/crtc: Add a generic infrastructure to fake VBLANK events
On Mon, 2 Jul 2018 10:37:22 +0200 Daniel Vetter wrote: > On Mon, Jul 02, 2018 at 10:14:51AM +0200, Boris Brezillon wrote: > > On Mon, 2 Jul 2018 10:02:52 +0200 > > Daniel Vetter wrote: > > > > > On Fri, Jun 29, 2018 at 01:17:17PM +0200, Boris Brezillon wrote: > > > > 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 > > > > that has no job queued. In this situation the CRTC is usually stopped > > > > until a new job is queued, and this can lead to timeouts when part of > > > > the pipeline is updated but no new jobs are queued to the active > > > > writeback connector. > > > > > > > > In order to solve that, we add a ->no_vblank flag to drm_crtc_state > > > > and ask the CRTC drivers to set it to true when they know they're not > > > > able to generate VBLANK events. The core drm_atomic_helper_fake_vblank() > > > > helper can then be used to fake VBLANKs at commit time. > > > > > > > > Signed-off-by: Boris Brezillon > > > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 40 > > > > + > > > > include/drm/drm_atomic_helper.h | 1 + > > > > include/drm/drm_crtc.h | 15 ++ > > > > 3 files changed, 56 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > > > b/drivers/gpu/drm/drm_atomic_helper.c > > > > index 69063bcf2334..ca586993c2a2 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > @@ -2051,6 +2051,46 @@ void > > > > drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state > > > > *old_state) > > > > } > > > > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > > > > > > > +/** > > > > + * drm_atomic_helper_fake_vblank - fake VBLANK events if needed > > > > + * @old_state: atomic state object with old state structures > > > > + * > > > > + * This function walks all CRTCs and fake VBLANK events on those with > > > > + * &drm_crtc_state.no_vblank set to true and &drm_crtc_state.event != > > > > NULL. > > > > + * The primary use of this function is writeback connectors working in > > > > oneshot > > > > + * mode and faking VBLANK events. In this case they only fake the > > > > VBLANK event > > > > + * when a job is queued, and any change to the pipeline that does not > > > > touch the > > > > + * connector is leading to timeouts when calling > > > > + * drm_atomic_helper_wait_for_vblanks() or > > > > + * drm_atomic_helper_wait_for_flip_done(). > > > > + * > > > > + * This is part of the atomic helper support for nonblocking commits, > > > > see > > > > + * drm_atomic_helper_setup_commit() for an overview. > > > > + */ > > > > +void drm_atomic_helper_fake_vblank(struct drm_atomic_state *old_state) > > > > +{ > > > > + struct drm_crtc_state *old_crtc_state, *new_crtc_state; > > > > + struct drm_crtc *crtc; > > > > + int i; > > > > + > > > > + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > > > > + new_crtc_state, i) { > > > > + unsigned long flags; > > > > + > > > > + if (!new_crtc_state->no_vblank && > > > > !old_crtc_state->no_vblank) > > > > > > Uh, this essentially makes it impossible to reset no_vblank. > > > > I don't want ->no_vblank to be reset by the core. It's up to the CRTC > > driver to clear/set it when something changes in the pipeline. > > > > > For control > > > flow state bits we only check the new state for it (see e.g. the various > > > *_changed or plane_bits or whatever). > > > > I tried with !new_crtc_state->no_vblank only, but then it does not > > handle the case where the CRTC and connector are being disabled, and I > > end up with a timeout. > > Why that? You should have a new_crtc_state even when you disable the crtc, > and you can set the ->no_vblank on that one too. Hm, right. I'll have to check why I didn't have ->no_blank set to true when I disable the CRTC. Probably a problem in vc4_crtc.c. -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel