[PATCH] drm/simple-helpers: Always add planes to the state update

[Noralf Trønnes]

Den 25.08.2016 08:30, skrev Daniel Vetter:
> On Wed, Aug 24, 2016 at 08:38:37PM +0200, Noralf Trønnes wrote:
>> Den 23.08.2016 08:25, skrev Daniel Vetter:
>>> Our update function is hooked to the single plane, which might not get
>>> called for crtc-only updates. Which is surprising, so fix this by
>>> always adding the plane.
>>>
>>> While at it document how&when the event should be sent out better in
>>> the kerneldoc.
>>>
>>> Cc: Noralf Trønnes <noralf at tronnes.org>
>>> Cc: andrea.merello at gmail.com
>>> Tested-and-Reported-by: andrea.merello at gmail.com
>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>> ---
>>> I'm not entirely sure we really want to put the responsibility for
>>> this onto driver. Plan B) would be to remove the kerneldoc I added
>>> here and call the right function from drm_simple_kms_plane_atomic_update.
>>> That way simple drivers don't need to deal with that detail, and in
>>> general those drivers don't care that much about the miniscule
>>> possible race a generic implementation would cause. What do you
>>> suggest as the best approach?
>> If the driver is responsible, a helper like this would be nice:
>>
>> void drm_simple_display_pipe_handle_vblank_event(struct
>> drm_simple_display_pipe *pipe)
>> {
>>      struct drm_crtc *crtc = &pipe->crtc;
>>
>>      if (crtc->state && crtc->state->event) {
>>          spin_lock_irq(&crtc->dev->event_lock);
>>          drm_crtc_send_vblank_event(crtc, crtc->state->event);
>>          spin_unlock_irq(&crtc->dev->event_lock);
>>          crtc->state->event = NULL;
> For a generic version we probably want to automatically switch to
> drm_crtc_arm_vblank_event, if vblank support is available. That can be
> checked by looking at drm_device->num_crtcs.
>
>>      }
>> }
>>
>> Then if the update() callback is not set, drm_simple_kms_plane_atomic_update
>> calls this, and if it is set, then the driver has to do it or something
>> similar.
>>
>> It is difficult to predict what future drivers will need.
>> Unless there are drivers on the horizon that will need to handle the
>> event themselves, I suggest that we follow your plan B. If the need arises,
>> we just add a one-liner to all drivers that has the update() callback set.
> Hm yeah. What about merging this patch here for now (would need your
> formal reviewed-by), and then later on when we have 2-3 simple drivers
> using this we can reconsider? Since I dont have hw for any of them I can't
> do this myself.
>
>> Noralf.
>>
>> Sidenote:
>> When I converted simpledrm I had problems with flip_done timeouts.
>> In the end I had to check for the event in disable/enable/update.
>> Trying it again now, it's enough to do it in update().
>> Not sure what has changed.
> Trying again with this patch, or without? Since this patch here is meant
> to exactly the problem that you need to handle the event in too many
> places. If it's fixed with just latest drm-misc, that would be surprising
> ...

I have updated Raspian so I could use xorg to test with, and indeed
without this patch and without sending vblank event in disable and enable,
the flip_done timeout showed up. modetest -v didn't trigger it.
With this patch I only have to send the vblank event in update().

Reviewed-by: Noralf Trønnes <noralf at tronnes.org>

> -Daniel
>
>>> -Daniel
>>> ---
>>>    drivers/gpu/drm/drm_simple_kms_helper.c | 7 +++++++
>>>    include/drm/drm_simple_kms_helper.h     | 6 ++++++
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> index bada17166512..447631018426 100644
>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> @@ -34,6 +34,12 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>>>    	.destroy = drm_encoder_cleanup,
>>>    };
>>> +static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>>> +				     struct drm_crtc_state *state)
>>> +{
>>> +	return drm_atomic_add_affected_planes(state->state, crtc);
>>> +}
>>> +
>>>    static void drm_simple_kms_crtc_enable(struct drm_crtc *crtc)
>>>    {
>>>    	struct drm_simple_display_pipe *pipe;
>>> @@ -57,6 +63,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc)
>>>    }
>>>    static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
>>> +	.atomic_check = drm_simple_kms_crtc_check,
>>>    	.disable = drm_simple_kms_crtc_disable,
>>>    	.enable = drm_simple_kms_crtc_enable,
>>>    };
>>> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
>>> index 269039722f91..826946ca2b82 100644
>>> --- a/include/drm/drm_simple_kms_helper.h
>>> +++ b/include/drm/drm_simple_kms_helper.h
>>> @@ -60,6 +60,12 @@ struct drm_simple_display_pipe_funcs {
>>>    	 *
>>>    	 * This function is called when the underlying plane state is updated.
>>>    	 * This hook is optional.
>>> +	 *
>>> +	 * This is the function drivers should submit the
>>> +	 * &drm_pending_vblank_event from. Using either
>>> +	 * drm_crtc_arm_vblank_event(), when the driver supports vblank
>>> +	 * interrupt handling, or drm_crtc_send_vblank_event() directly in case
>>> +	 * the hardware lacks vblank support entirely.
>>>    	 */
>>>    	void (*update)(struct drm_simple_display_pipe *pipe,
>>>    		       struct drm_plane_state *plane_state);