Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Zimmermann <tzimmermann@suse.de>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: david@lechnology.com, oleksandr_andrushchenko@epam.com,
	airlied@linux.ie, sam@ravnborg.org,
	dri-devel@lists.freedesktop.org,
	maarten.lankhorst@linux.intel.com, mripard@kernel.org,
	virtualization@lists.linux-foundation.org, hdegoede@redhat.com,
	noralf@tronnes.org, kraxel@redhat.com,
	xen-devel@lists.xenproject.org, emil.velikov@collabora.com,
	sean@poorly.run, laurent.pinchart@ideasonboard.com
Subject: Re: [Xen-devel] [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default
Date: Thu, 16 Jan 2020 08:37:26 +0100
Message-ID: <33fdd33f-ce8d-70d3-544e-fac727d2686b@suse.de> (raw)
In-Reply-To: <20200116064107.GB8400@dvetter-linux.ger.corp.intel.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 4523 bytes --]

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@suse.de>
> 
>> 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-15 12:52 [Xen-devel] [PATCH v2 0/4] Use no_vblank property for drivers without VBLANK Thomas Zimmermann
2020-01-15 12:52 ` [Xen-devel] [PATCH v2 1/4] drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events Thomas Zimmermann
2020-01-15 12:52 ` [Xen-devel] [PATCH v2 2/4] drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check() Thomas Zimmermann
2020-01-15 12:52 ` [Xen-devel] [PATCH v2 3/4] drm/cirrus: Let DRM core send VBLANK events Thomas Zimmermann
2020-01-15 12:52 ` [Xen-devel] [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default Thomas Zimmermann
2020-01-16  6:41   ` Daniel Vetter
2020-01-16  7:37     ` Thomas Zimmermann [this message]
2020-01-16 17:22       ` Emil Velikov
2020-01-16 23:59         ` Daniel Vetter
2020-01-17  7:17           ` Thomas Zimmermann
2020-01-22  8:11             ` Daniel Vetter
2020-01-22  8:20               ` Thomas Zimmermann
2020-01-15 13:04 ` [Xen-devel] [PATCH v2 0/4] Use no_vblank property for drivers without VBLANK Hans de Goede
  -- strict thread matches above, loose matches on Subject: below --
2020-01-13 10:38 Thomas Zimmermann
2020-01-13 10:38 ` [Xen-devel] [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default Thomas Zimmermann

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=33fdd33f-ce8d-70d3-544e-fac727d2686b@suse.de \
    --to=tzimmermann@suse.de \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=david@lechnology.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=emil.velikov@collabora.com \
    --cc=hdegoede@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mripard@kernel.org \
    --cc=noralf@tronnes.org \
    --cc=oleksandr_andrushchenko@epam.com \
    --cc=sam@ravnborg.org \
    --cc=sean@poorly.run \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git