diff mbox series

[v4,15/15] drm/xen: Explicitly disable automatic sending of vblank event

Message ID 20200123092123.28368-16-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series Use no_vblank property for drivers without VBLANK | expand

Commit Message

Thomas Zimmermann Jan. 23, 2020, 9:21 a.m. UTC 
The atomic helpers automatically send out fake VBLANK events if no
vblanking has been initialized. This would apply to xen, but xen has
its own vblank logic. To avoid interfering with the atomic helpers,
disable automatic vblank events explictly.

v4:
	* separate commit from core vblank changes

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Daniel Vetter Jan. 27, 2020, 9:47 a.m. UTC | #1 
On Thu, Jan 23, 2020 at 10:21:23AM +0100, Thomas Zimmermann wrote:
> The atomic helpers automatically send out fake VBLANK events if no
> vblanking has been initialized. This would apply to xen, but xen has
> its own vblank logic. To avoid interfering with the atomic helpers,
> disable automatic vblank events explictly.
> 
> v4:
> 	* separate commit from core vblank changes
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

On all the driver patches:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 4f34c5208180..efde4561836f 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -220,6 +220,18 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
>  	return false;
>  }
>  
> +static int display_check(struct drm_simple_display_pipe *pipe,
> +			 struct drm_plane_state *plane_state,
> +			 struct drm_crtc_state *crtc_state)
> +{
> +	/* Make sure that DRM helpers don't send VBLANK events
> +	 * automatically. Xen has it's own logic to do so.
> +	 */
> +	crtc_state->no_vblank = false;

So this here is strictly speaking dead code, because the fake_vblank
helper makes sure to only send out the event if it hasn't been processed
yet.

Which is a bit awkward, since it does this silently, potentially hiding
bugs. We already have a warning that complains if a crtc_state->event
hasn't been processed at all. But with the auto-vblank stuff here we kinda
defeat that a bit ... I think adding a WARN_ON in fake_vblank that fires
if no_vblank is set, but the event is somehow gone, would be really
useful. That would point at some serious driver snafu ...

Care to throw that on top?
-Daniel

> +
> +	return 0;
> +}
> +
>  static void display_update(struct drm_simple_display_pipe *pipe,
>  			   struct drm_plane_state *old_plane_state)
>  {
> @@ -284,6 +296,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
>  	.enable = display_enable,
>  	.disable = display_disable,
>  	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> +	.check = display_check,
>  	.update = display_update,
>  };
>  
> -- 
> 2.24.1
>
Oleksandr Andrushchenko Jan. 27, 2020, 9:53 a.m. UTC | #2 
Sorry for jumping in late

On 1/23/20 11:21 AM, Thomas Zimmermann wrote:
> The atomic helpers automatically send out fake VBLANK events if no
> vblanking has been initialized. This would apply to xen, but xen has
> its own vblank logic. To avoid interfering with the atomic helpers,
> disable automatic vblank events explictly.
>
> v4:
> 	* separate commit from core vblank changes
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

> ---
>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 4f34c5208180..efde4561836f 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> @@ -220,6 +220,18 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
>   	return false;
>   }
>   
> +static int display_check(struct drm_simple_display_pipe *pipe,
> +			 struct drm_plane_state *plane_state,
> +			 struct drm_crtc_state *crtc_state)
> +{
> +	/* Make sure that DRM helpers don't send VBLANK events
Could you please put the comment on a separate line?
> +	 * automatically. Xen has it's own logic to do so.
> +	 */
> +	crtc_state->no_vblank = false;
And it is still confusing, e.g. comment says
"Make sure that DRM helpers don't send VBLANK"
and we set "no_vblank" flag to false...
> +
> +	return 0;
> +}
> +
>   static void display_update(struct drm_simple_display_pipe *pipe,
>   			   struct drm_plane_state *old_plane_state)
>   {
> @@ -284,6 +296,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
>   	.enable = display_enable,
>   	.disable = display_disable,
>   	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> +	.check = display_check,
>   	.update = display_update,
>   };
>
Thomas Zimmermann Jan. 27, 2020, 11:59 a.m. UTC | #3 
Hi

Am 27.01.20 um 10:53 schrieb Oleksandr Andrushchenko:
> Sorry for jumping in late
> 
> On 1/23/20 11:21 AM, Thomas Zimmermann wrote:
>> The atomic helpers automatically send out fake VBLANK events if no
>> vblanking has been initialized. This would apply to xen, but xen has
>> its own vblank logic. To avoid interfering with the atomic helpers,
>> disable automatic vblank events explictly.
>>
>> v4:
>> 	* separate commit from core vblank changes
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
>> ---
>>   drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> index 4f34c5208180..efde4561836f 100644
>> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>> @@ -220,6 +220,18 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
>>   	return false;
>>   }
>>   
>> +static int display_check(struct drm_simple_display_pipe *pipe,
>> +			 struct drm_plane_state *plane_state,
>> +			 struct drm_crtc_state *crtc_state)
>> +{
>> +	/* Make sure that DRM helpers don't send VBLANK events
> Could you please put the comment on a separate line?

You mean to add an empty line between comment and code?

>> +	 * automatically. Xen has it's own logic to do so.
>> +	 */
>> +	crtc_state->no_vblank = false;
> And it is still confusing, e.g. comment says
> "Make sure that DRM helpers don't send VBLANK"
> and we set "no_vblank" flag to false...

I'll rephrase and add some more context.

Best regards
Thomas

>> +
>> +	return 0;
>> +}
>> +
>>   static void display_update(struct drm_simple_display_pipe *pipe,
>>   			   struct drm_plane_state *old_plane_state)
>>   {
>> @@ -284,6 +296,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
>>   	.enable = display_enable,
>>   	.disable = display_disable,
>>   	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>> +	.check = display_check,
>>   	.update = display_update,
>>   };
>>
Oleksandr Andrushchenko Jan. 27, 2020, 12:10 p.m. UTC | #4 
On 1/27/20 1:59 PM, Thomas Zimmermann wrote:
> Hi
>
> Am 27.01.20 um 10:53 schrieb Oleksandr Andrushchenko:
>> Sorry for jumping in late
>>
>> On 1/23/20 11:21 AM, Thomas Zimmermann wrote:
>>> The atomic helpers automatically send out fake VBLANK events if no
>>> vblanking has been initialized. This would apply to xen, but xen has
>>> its own vblank logic. To avoid interfering with the atomic helpers,
>>> disable automatic vblank events explictly.
>>>
>>> v4:
>>> 	* separate commit from core vblank changes
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>>> ---
>>>    drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++++
>>>    1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>>> index 4f34c5208180..efde4561836f 100644
>>> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
>>> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
>>> @@ -220,6 +220,18 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
>>>    	return false;
>>>    }
>>>    
>>> +static int display_check(struct drm_simple_display_pipe *pipe,
>>> +			 struct drm_plane_state *plane_state,
>>> +			 struct drm_crtc_state *crtc_state)
>>> +{
>>> +	/* Make sure that DRM helpers don't send VBLANK events
>> Could you please put the comment on a separate line?
> You mean to add an empty line between comment and code?
>
Just like
/*
  * Make sure...
>>> +	 * automatically. Xen has it's own logic to do so.
>>> +	 */
>>> +	crtc_state->no_vblank = false;
>> And it is still confusing, e.g. comment says
>> "Make sure that DRM helpers don't send VBLANK"
>> and we set "no_vblank" flag to false...
> I'll rephrase and add some more context.
Thank you
>
> Best regards
> Thomas
>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    static void display_update(struct drm_simple_display_pipe *pipe,
>>>    			   struct drm_plane_state *old_plane_state)
>>>    {
>>> @@ -284,6 +296,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = {
>>>    	.enable = display_enable,
>>>    	.disable = display_disable,
>>>    	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>>> +	.check = display_check,
>>>    	.update = display_update,
>>>    };
>>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 4f34c5208180..efde4561836f 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
@@ -220,6 +220,18 @@  static bool display_send_page_flip(struct drm_simple_display_pipe *pipe,
 	return false;
 }
 
+static int display_check(struct drm_simple_display_pipe *pipe,
+			 struct drm_plane_state *plane_state,
+			 struct drm_crtc_state *crtc_state)
+{
+	/* Make sure that DRM helpers don't send VBLANK events
+	 * automatically. Xen has it's own logic to do so.
+	 */
+	crtc_state->no_vblank = false;
+
+	return 0;
+}
+
 static void display_update(struct drm_simple_display_pipe *pipe,
 			   struct drm_plane_state *old_plane_state)
 {
@@ -284,6 +296,7 @@  static const struct drm_simple_display_pipe_funcs display_funcs = {
 	.enable = display_enable,
 	.disable = display_disable,
 	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+	.check = display_check,
 	.update = display_update,
 };