[RFC] HACK: drm_atomic: Don't try to set vblank if crtc isn't active

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(+)

-- 
2.7.4

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