[PATCH] drm_fb_helper: fix regression in pixclock check

[PATCH] drm_fb_helper: fix regression in pixclock check

[Jason Wessel]

Commit 5349ef3127c77075ff70b2014f17ae0fbcaaf199 changed pixclock to be
initialized to zero instead of -1.  The validation routine always
returns -EINVAL for a valid pixclock which prevents atomic kernel mode
setting from working correctly.

CC: David Airlie <airlied@linux.ie>
CC: Jesse Barnes <jbarnes@virtuousgeek.org>
CC: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/gpu/drm/drm_fb_helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1b49fa0..7b59cf1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -602,7 +602,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	struct drm_framebuffer *fb = fb_helper->fb;
 	int depth;
 
-	if (var->pixclock != 0)
+	if (!var->pixclock)
 		return -EINVAL;
 
 	/* Need to resize the fb object !!! */
-- 
1.6.4.rc1

Re: [PATCH] drm_fb_helper: fix regression in pixclock check

[Clemens Ladisch]

Jason Wessel wrote:
> Commit 5349ef3127c77075ff70b2014f17ae0fbcaaf199 changed pixclock to be
> initialized to zero instead of -1.

Zero meaning "there is no pixel clock value".  When I wrote the patch,
the FB (helper) code did not use this value at all, so zero was the only
value that could possibly be used.

> The validation routine always returns -EINVAL for a valid pixclock

It declares that the only valid pixclock value is "none".  This was
required to make the userspace API work, since the GET ioctls return
this value, and all programs expect to be able to PUT this value back.

> which prevents atomic kernel mode setting from working correctly.
> ...
> @@ -602,7 +602,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  
> -	if (var->pixclock != 0)
> +	if (!var->pixclock)
>  		return -EINVAL;

This change breaks the userspace API again, unless the other parts of
the code have been changed to always return a valid pixclock value when
reading the current mode settings.

If we want to accept both zero and non-zero values for pixclock, this
check should be dropped altogether.

In drm_fb_helper_set_par(), there is a corresponding check that must be
kept in sync with this one.


Best regards,
Clemens

Re: [PATCH] drm_fb_helper: fix regression in pixclock check

[Jason Wessel]

Clemens Ladisch wrote:
> Jason Wessel wrote:
>   
>> Commit 5349ef3127c77075ff70b2014f17ae0fbcaaf199 changed pixclock to be
>> initialized to zero instead of -1.
>>     
>
> Zero meaning "there is no pixel clock value".  When I wrote the patch,
> the FB (helper) code did not use this value at all, so zero was the only
> value that could possibly be used.
>
>   
>> The validation routine always returns -EINVAL for a valid pixclock
>>     
>
> It declares that the only valid pixclock value is "none".  This was
> required to make the userspace API work, since the GET ioctls return
> this value, and all programs expect to be able to PUT this value back.
>
>   
>> which prevents atomic kernel mode setting from working correctly.
>> ...
>> @@ -602,7 +602,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>>  
>> -	if (var->pixclock != 0)
>> +	if (!var->pixclock)
>>  		return -EINVAL;
>>     
>
> This change breaks the userspace API again, unless the other parts of
> the code have been changed to always return a valid pixclock value when
> reading the current mode settings.
>
> If we want to accept both zero and non-zero values for pixclock, this
> check should be dropped altogether.
>
> In drm_fb_helper_set_par(), there is a corresponding check that must be
> kept in sync with this one.
>
>   

Fair enough.  We'll make a special case for when the kernel debugger is
active.

Looks to me like it worked by luck during the 2.6.32 kernel cycle for
the atomic kernel mode setting.  I have changed it in the next version
of this patch to always return -EINVAL while the kernel debugger is
active.  This restores the atomic kernel mode setting back to the
working state, until we can find a better solution.

Thanks,
Jason.