| Message ID | 1516696258-32183-1-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 23, 2018 at 12:30:58AM -0800, John Stultz wrote: > In trying to use the drm_hwcomposer on HiKey, I found things > wouldn't initialize due to the following check in > drm_atomic_crtc_check() failing: > > if (state->event && !state->active && !crtc->state->active) { > DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n", > crtc->base.id, crtc->name); > return -EINVAL; > } I think the answer is in the comment directly above this code. Having an event present while the crtc _stays_ off is the problem. So drm_hwc is requesting an event (or providing a fence) for a crtc which it does not turn on. So I think you should go back into hwc and find out how this situation arises. AFAIK, requesting an event in a commit with both a modeset and plane update should work. Sean > > This case fails because the crtc_state hasn't been set to active > yet. > > However, this trips on the first call to drm_atomic_commit(), > where the drm_hwcomposer is trying to setup the initial mode. > > Digging further its seems the state->event value is set because > before we get into handling the atomic mode, we call > prepare_crtc_signaling(), which sees a fence ptr, and calls > create_vblank_event(). > > So to hack around this, I've added a check in > prepare_crtc_signaling() to see if the crtc_state is active and > if not, skip trying to create the vblank event. > > I suspect this isn't correct, but I'm a bit confused on what the > right solution is. I was thinking the drm_hwcomposer was missing > something to enable the display before calling > drmModeAtomicCommit(), but as I look at the logic it seems that > should be ok. I'm starting to suspect I only see this issue > because I don't have the framebuffer console enabled, so the > kernel has yet to initialize the crtc, and that's probably not > commonly used. > > Any thoughts or feedback on a better approach to solving this > issue would be greatly appreciated! > > Cc: Marissa Wall <marissaw@google.com> > Cc: Sean Paul <seanpaul@google.com> > Cc: Dmitry Shmidt <dimitrysh@google.com> > Cc: Matt Szczesiak <matt.szczesiak@arm.com> > Cc: Liviu Dudau <Liviu.Dudau@arm.com> > Cc: David Hanna <david.hanna11@gmail.com> > Cc: Rob Herring <rob.herring@linaro.org> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Robert Foss <robert.foss@collabora.com> > Cc: Gustavo Padovan <gustavo.padovan@collabora.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/gpu/drm/drm_atomic.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index c2da558..e6404b2 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2072,6 +2072,9 @@ static int prepare_crtc_signaling(struct drm_device *dev, > for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > s32 __user *fence_ptr; > > + if (!crtc_state->active) > + continue; > + > fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc); > > if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) { > -- > 2.7.4 >
On Tue, Jan 23, 2018 at 7:58 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, Jan 23, 2018 at 12:30:58AM -0800, John Stultz wrote: >> In trying to use the drm_hwcomposer on HiKey, I found things >> wouldn't initialize due to the following check in >> drm_atomic_crtc_check() failing: >> >> if (state->event && !state->active && !crtc->state->active) { >> DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n", >> crtc->base.id, crtc->name); >> return -EINVAL; >> } > > I think the answer is in the comment directly above this code. Having an event > present while the crtc _stays_ off is the problem. So drm_hwc is requesting an > event (or providing a fence) for a crtc which it does not turn on. So I think > you should go back into hwc and find out how this situation arises. So in this case, its providing a fence which causes the event to be set. I'll look some more, but it seems that we call this check (drm_atomic_check_only) before we do the commit apply the modeset that would turn on the crtc. Thus the check fails and we don't do the commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_atomic.c#n1659 Just commenting out the EINVAL in the snippet above causes the crtc to start up fine. So it seems like either the first modeset/plane update shouldn't be done along w/ a fence, or the kernel should maybe skip setting the event? > AFAIK, requesting an event in a commit with both a modeset and plane update > should work. That's what it looks like to me too, which is why I'm confused. thanks -john
On Tue, Jan 23, 2018 at 2:23 PM, John Stultz <john.stultz@linaro.org> wrote: > On Tue, Jan 23, 2018 at 7:58 AM, Sean Paul <seanpaul@chromium.org> wrote: >> On Tue, Jan 23, 2018 at 12:30:58AM -0800, John Stultz wrote: >>> In trying to use the drm_hwcomposer on HiKey, I found things >>> wouldn't initialize due to the following check in >>> drm_atomic_crtc_check() failing: >>> >>> if (state->event && !state->active && !crtc->state->active) { >>> DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n", >>> crtc->base.id, crtc->name); >>> return -EINVAL; >>> } >> >> I think the answer is in the comment directly above this code. Having an event >> present while the crtc _stays_ off is the problem. So drm_hwc is requesting an >> event (or providing a fence) for a crtc which it does not turn on. So I think >> you should go back into hwc and find out how this situation arises. > > So in this case, its providing a fence which causes the event to be set. > > I'll look some more, but it seems that we call this check > (drm_atomic_check_only) before we do the commit apply the modeset that > would turn on the crtc. > Thus the check fails and we don't do the commit: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_atomic.c#n1659 > > Just commenting out the EINVAL in the snippet above causes the crtc to > start up fine. > > So it seems like either the first modeset/plane update shouldn't be > done along w/ a fence, or the kernel should maybe skip setting the > event? > I'd tend to focus more on why the crtc is not active in the new state. Which is something that's specified in the atomic commit coming from hwc, right? Sean >> AFAIK, requesting an event in a commit with both a modeset and plane update >> should work. > > That's what it looks like to me too, which is why I'm confused. > > thanks > -john
On Tue, Jan 23, 2018 at 12:44 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, Jan 23, 2018 at 2:23 PM, John Stultz <john.stultz@linaro.org> wrote: >> On Tue, Jan 23, 2018 at 7:58 AM, Sean Paul <seanpaul@chromium.org> wrote: >>> On Tue, Jan 23, 2018 at 12:30:58AM -0800, John Stultz wrote: >>>> In trying to use the drm_hwcomposer on HiKey, I found things >>>> wouldn't initialize due to the following check in >>>> drm_atomic_crtc_check() failing: >>>> >>>> if (state->event && !state->active && !crtc->state->active) { >>>> DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n", >>>> crtc->base.id, crtc->name); >>>> return -EINVAL; >>>> } >>> >>> I think the answer is in the comment directly above this code. Having an event >>> present while the crtc _stays_ off is the problem. So drm_hwc is requesting an >>> event (or providing a fence) for a crtc which it does not turn on. So I think >>> you should go back into hwc and find out how this situation arises. >> >> So in this case, its providing a fence which causes the event to be set. >> >> I'll look some more, but it seems that we call this check >> (drm_atomic_check_only) before we do the commit apply the modeset that >> would turn on the crtc. >> Thus the check fails and we don't do the commit: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_atomic.c#n1659 >> >> Just commenting out the EINVAL in the snippet above causes the crtc to >> start up fine. >> >> So it seems like either the first modeset/plane update shouldn't be >> done along w/ a fence, or the kernel should maybe skip setting the >> event? >> > > I'd tend to focus more on why the crtc is not active in the new state. > Which is something that's specified in the atomic commit coming from > hwc, right? Ah. Sounds like I've been mixing up the kernel's state of the hardware with the requested state to be committed. Many thanks for the clarification. I'll dig a bit further. thanks -john
On Tue, Jan 23, 2018 at 12:44 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, Jan 23, 2018 at 2:23 PM, John Stultz <john.stultz@linaro.org> wrote: >> On Tue, Jan 23, 2018 at 7:58 AM, Sean Paul <seanpaul@chromium.org> wrote: >>> On Tue, Jan 23, 2018 at 12:30:58AM -0800, John Stultz wrote: >>>> In trying to use the drm_hwcomposer on HiKey, I found things >>>> wouldn't initialize due to the following check in >>>> drm_atomic_crtc_check() failing: >>>> >>>> if (state->event && !state->active && !crtc->state->active) { >>>> DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n", >>>> crtc->base.id, crtc->name); >>>> return -EINVAL; >>>> } >>> >>> I think the answer is in the comment directly above this code. Having an event >>> present while the crtc _stays_ off is the problem. So drm_hwc is requesting an >>> event (or providing a fence) for a crtc which it does not turn on. So I think >>> you should go back into hwc and find out how this situation arises. >> >> So in this case, its providing a fence which causes the event to be set. >> >> I'll look some more, but it seems that we call this check >> (drm_atomic_check_only) before we do the commit apply the modeset that >> would turn on the crtc. >> Thus the check fails and we don't do the commit: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_atomic.c#n1659 >> >> Just commenting out the EINVAL in the snippet above causes the crtc to >> start up fine. >> >> So it seems like either the first modeset/plane update shouldn't be >> done along w/ a fence, or the kernel should maybe skip setting the >> event? >> > > I'd tend to focus more on why the crtc is not active in the new state. > Which is something that's specified in the atomic commit coming from > hwc, right? Yes! hwc isn't setting the active property, I'll send a fix to drm_hwcomposer soon. Thanks so much again for the guidance here! thanks -john
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index c2da558..e6404b2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2072,6 +2072,9 @@ static int prepare_crtc_signaling(struct drm_device *dev, for_each_new_crtc_in_state(state, crtc, crtc_state, i) { s32 __user *fence_ptr; + if (!crtc_state->active) + continue; + fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc); if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
In trying to use the drm_hwcomposer on HiKey, I found things wouldn't initialize due to the following check in drm_atomic_crtc_check() failing: if (state->event && !state->active && !crtc->state->active) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n", crtc->base.id, crtc->name); return -EINVAL; } This case fails because the crtc_state hasn't been set to active yet. However, this trips on the first call to drm_atomic_commit(), where the drm_hwcomposer is trying to setup the initial mode. Digging further its seems the state->event value is set because before we get into handling the atomic mode, we call prepare_crtc_signaling(), which sees a fence ptr, and calls create_vblank_event(). So to hack around this, I've added a check in prepare_crtc_signaling() to see if the crtc_state is active and if not, skip trying to create the vblank event. I suspect this isn't correct, but I'm a bit confused on what the right solution is. I was thinking the drm_hwcomposer was missing something to enable the display before calling drmModeAtomicCommit(), but as I look at the logic it seems that should be ok. I'm starting to suspect I only see this issue because I don't have the framebuffer console enabled, so the kernel has yet to initialize the crtc, and that's probably not commonly used. Any thoughts or feedback on a better approach to solving this issue would be greatly appreciated! Cc: Marissa Wall <marissaw@google.com> Cc: Sean Paul <seanpaul@google.com> Cc: Dmitry Shmidt <dimitrysh@google.com> Cc: Matt Szczesiak <matt.szczesiak@arm.com> Cc: Liviu Dudau <Liviu.Dudau@arm.com> Cc: David Hanna <david.hanna11@gmail.com> Cc: Rob Herring <rob.herring@linaro.org> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Robert Foss <robert.foss@collabora.com> Cc: Gustavo Padovan <gustavo.padovan@collabora.com> Cc: Rob Clark <robdclark@gmail.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/gpu/drm/drm_atomic.c | 3 +++ 1 file changed, 3 insertions(+)