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);
