Re: [Xen-devel] [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 16.01.20 um 07:41 schrieb Daniel Vetter:
> On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote:
>> In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
>> if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.
>>
>> For drivers that have neither an enable_vblank() callback nor a check()
>> callback, the simple-KMS helpers enable VBLANK generation by default. This
>> simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
>> DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
>> its own logic for sending these events.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> 
>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
>> b/drivers/gpu/drm/drm_simple_kms_helper.c
>> index 15fb516ae2d8..4414c7a5b2ce 100644
>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>> @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct 
>> drm_plane *plane,
>>      if (!plane_state->visible)
>>              return 0;
>>  
>> -    if (!pipe->funcs || !pipe->funcs->check)
>> -            return 0;
>> -
>> -    return pipe->funcs->check(pipe, plane_state, crtc_state);
>> +    if (pipe->funcs) {
>> +            if (pipe->funcs->check)
>> +                    return pipe->funcs->check(pipe, plane_state,
>> +                                              crtc_state);
>> +            if (pipe->funcs->enable_vblank)
>> +                    return 0;
>> +    }
>> +
>> +    /* Drivers without VBLANK support have to fake VBLANK events. As
>> +     * there's no check() callback to enable this, set the no_vblank
>> +     * field by default.
>> +     */
> 
> The ->check callback is right above this comment ... I'm confused.

I guess that comment isn't overly precise. What it means is that
no_vblank would have to be set in check(), but the driver did not
specify a check() function. So it has neither vblank support nor any way
of setting no_vblank. Hence, the simple-kms helper sets no_vblank
automatically.

Maybe something to update for the patchset's v2.

> 
>> +    crtc_state->no_vblank = true;
> 
> That's kinda not what I meant with handling this automatically. Instead
> something like this:
> 
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 7cf3cf936547..23d2f51fc1d4 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct 
> drm_crtc *crtc,
>       /* Self refresh should be canceled when a new update is available */
>       state->active = drm_atomic_crtc_effectively_active(state);
>       state->self_refresh_active = false;
> +
> +     if (drm_dev_has_vblank(crtc->dev))
> +             state->no_vblank = true;
> +     else
> +             state->no_vblank = false;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);

I think the if/else branches are in the wrong order.

But generally speaking, is it really that easy? The xen driver already
has to work around simple-kms's auto-enabling of no_vblank (see patch
4). Maybe this settings interferes with other drivers as well. At least
the calls for sending fake vblanks should be removed from all affected
drivers.

Best regards
Thomas

>  
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 1659b13b178c..32cab3d3c872 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -81,6 +81,12 @@
>   */
>  #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
>  
> +/* FIXME roll this out here in this file */
> +bool drm_dev_has_vblank(dev)
> +{
> +     return dev->num_crtcs;
> +}
> +
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
>                         ktime_t *tvblank, bool in_vblank_irq);
> 
> 
> But maybe move the default value to some other/better place in the atomic
> helpers, not sure what the best one is.
> 
> Plus then in the documentation patch also highlight the link between
> crtc_state->no_vblank and drm_dev_has_vblank respectively
> drm_device.num_crtcs.
> 
> That should plug this issue once for all across the board.
> 
> There's still the fun between having the vblank callbacks and the
> drm_vblank setup, but that's a much older can of worms ...
> -Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel