[PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

Previously, the vmwgfx_fb driver would allow users to call FBIOSET_VINFO, but it would not adjust
the FINFO properly, resulting in distorted screen rendering. The patch corrects that behaviour.

See https://bugs.gentoo.org/show_bug.cgi?id=494794 for examples.

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index ed5ce2a..021b522 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -147,7 +147,7 @@ static int vmw_fb_check_var(struct fb_var_screeninfo *var,
 	}
 
 	if (!vmw_kms_validate_mode_vram(vmw_priv,
-					info->fix.line_length,
+					var->xres * var->bits_per_pixel/8,
 					var->yoffset + var->yres)) {
 		DRM_ERROR("Requested geom can not fit in framebuffer\n");
 		return -EINVAL;
@@ -162,6 +162,8 @@ static int vmw_fb_set_par(struct fb_info *info)
 	struct vmw_private *vmw_priv = par->vmw_priv;
 	int ret;
 
+	info->fix.line_length = info->var.xres * info->var.bits_per_pixel/8;
+
 	ret = vmw_kms_write_svga(vmw_priv, info->var.xres, info->var.yres,
 				 info->fix.line_length,
 				 par->bpp, par->depth);
@@ -177,6 +179,7 @@ static int vmw_fb_set_par(struct fb_info *info)
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_POSITION_Y, info->var.yoffset);
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_WIDTH, info->var.xres);
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_HEIGHT, info->var.yres);
+		vmw_write(vmw_priv, SVGA_REG_BYTES_PER_LINE, info->fix.line_length);
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_ID, SVGA_ID_INVALID);
 	}
 
-- 
1.8.3.2

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

This is the 2nd time I've posted the patch. Please have a look Dave.
Gregkh suggested I resend this a while back after not hearing any
feedback.

It's a fairly simple patch. Don't forget to look at

https://bugs.gentoo.org/show_bug.cgi?id=494794

because that kind of describes it all.

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Dave Airlie]

On Fri, Mar 28, 2014 at 10:45 AM, Christopher Friedt
<chrisfriedt@gmail.com> wrote:
> Previously, the vmwgfx_fb driver would allow users to call FBIOSET_VINFO, but it would not adjust
> the FINFO properly, resulting in distorted screen rendering. The patch corrects that behaviour.
>
> See https://bugs.gentoo.org/show_bug.cgi?id=494794 for examples.
>

Just adding cc's of maintainer list.

> Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index ed5ce2a..021b522 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -147,7 +147,7 @@ static int vmw_fb_check_var(struct fb_var_screeninfo *var,
>         }
>
>         if (!vmw_kms_validate_mode_vram(vmw_priv,
> -                                       info->fix.line_length,
> +                                       var->xres * var->bits_per_pixel/8,
>                                         var->yoffset + var->yres)) {
>                 DRM_ERROR("Requested geom can not fit in framebuffer\n");
>                 return -EINVAL;
> @@ -162,6 +162,8 @@ static int vmw_fb_set_par(struct fb_info *info)
>         struct vmw_private *vmw_priv = par->vmw_priv;
>         int ret;
>
> +       info->fix.line_length = info->var.xres * info->var.bits_per_pixel/8;
> +
>         ret = vmw_kms_write_svga(vmw_priv, info->var.xres, info->var.yres,
>                                  info->fix.line_length,
>                                  par->bpp, par->depth);
> @@ -177,6 +179,7 @@ static int vmw_fb_set_par(struct fb_info *info)
>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_POSITION_Y, info->var.yoffset);
>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_WIDTH, info->var.xres);
>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_HEIGHT, info->var.yres);
> +               vmw_write(vmw_priv, SVGA_REG_BYTES_PER_LINE, info->fix.line_length);
>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_ID, SVGA_ID_INVALID);
>         }
>
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Thomas Hellstrom]

On 03/28/2014 02:45 AM, Dave Airlie wrote:
> On Fri, Mar 28, 2014 at 10:45 AM, Christopher Friedt
> <chrisfriedt@gmail.com> wrote:
>> Previously, the vmwgfx_fb driver would allow users to call FBIOSET_VINFO, but it would not adjust
>> the FINFO properly, resulting in distorted screen rendering. The patch corrects that behaviour.
>>
>> See https://bugs.gentoo.org/show_bug.cgi?id=494794 for examples.
>>
> Just adding cc's of maintainer list.
Looks correct to me.
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>

Will add it to vmgfx-next and cc stable.

Thanks,
Thomas



>> Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
>> ---
>>  drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> index ed5ce2a..021b522 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
>> @@ -147,7 +147,7 @@ static int vmw_fb_check_var(struct fb_var_screeninfo *var,
>>         }
>>
>>         if (!vmw_kms_validate_mode_vram(vmw_priv,
>> -                                       info->fix.line_length,
>> +                                       var->xres * var->bits_per_pixel/8,
>>                                         var->yoffset + var->yres)) {
>>                 DRM_ERROR("Requested geom can not fit in framebuffer\n");
>>                 return -EINVAL;
>> @@ -162,6 +162,8 @@ static int vmw_fb_set_par(struct fb_info *info)
>>         struct vmw_private *vmw_priv = par->vmw_priv;
>>         int ret;
>>
>> +       info->fix.line_length = info->var.xres * info->var.bits_per_pixel/8;
>> +
>>         ret = vmw_kms_write_svga(vmw_priv, info->var.xres, info->var.yres,
>>                                  info->fix.line_length,
>>                                  par->bpp, par->depth);
>> @@ -177,6 +179,7 @@ static int vmw_fb_set_par(struct fb_info *info)
>>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_POSITION_Y, info->var.yoffset);
>>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_WIDTH, info->var.xres);
>>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_HEIGHT, info->var.yres);
>> +               vmw_write(vmw_priv, SVGA_REG_BYTES_PER_LINE, info->fix.line_length);
>>                 vmw_write(vmw_priv, SVGA_REG_DISPLAY_ID, SVGA_ID_INVALID);
>>         }
>>
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Thomas Hellstrom]

On 06/30/2014 12:19 PM, Christopher Friedt wrote:
>
>
> On Apr 1, 2014 6:00 AM, "Thomas Hellstrom" <thellstrom@vmware.com
> <mailto:thellstrom@vmware.com>> wrote:
> >
> > On 03/28/2014 02:45 AM, Dave Airlie wrote:
> > > On Fri, Mar 28, 2014 at 10:45 AM, Christopher Friedt
> > > <chrisfriedt@gmail.com <mailto:chrisfriedt@gmail.com>> wrote:
> > >> Previously, the vmwgfx_fb driver would allow users to call
> FBIOSET_VINFO, but it would not adjust
> > >> the FINFO properly, resulting in distorted screen rendering. The
> patch corrects that behaviour.
> > >>
> > >> See https://bugs.gentoo.org/show_bug.cgi?id=494794
> <https://urldefense.proofpoint.com/v1/url?u=https://bugs.gentoo.org/show_bug.cgi?id%3D494794&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=hjf3nJVkhVmtW5ub%2FB2AFpKE1pI6pdX%2FvGBFan6861g%3D%0A&s=ca98e09efe2b59ab77e21ab77663e91400437f93d391e28139cb49aa0e21948c>
> for examples.
>
> This correction in vmwgfx appears to have triggered a bug in
> xorg-video-vmware on Ubuntu systems with a custom / unreleased kernel.
>
> If anyone from the X community does not step in, I'll take a look at
> it today.
>
> https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-vmware/+bug/1328898
> <https://urldefense.proofpoint.com/v1/url?u=https://bugs.launchpad.net/ubuntu/%2Bsource/xserver-xorg-video-vmware/%2Bbug/1328898&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=hjf3nJVkhVmtW5ub%2FB2AFpKE1pI6pdX%2FvGBFan6861g%3D%0A&s=56cf4e2efa552bd5eaa2fe7cd82af3f561d717bdd9b6745c84451e1ab6bd05d1>
>
> https://communities.vmware.com/message/2388776
>
+ vmware maintainers

I don't think we can blame video-vmware for this. A kernel driver change
that breaks existing user-space is by definition a kernel driver bug,
regardless whether exisiting user-space is doing something horrendously
stupid.

So the fix must IMO be a kernel driver fix. My initial guess is that
once we set the bytes per line register, it might not be automatically
updated when the screen width is changed, but the documentation is poor.
I see if I can shed some light over this.

/Thomas

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

On Mon, Jun 30, 2014 at 7:48 AM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> I don't think we can blame video-vmware for this. A kernel driver change
> that breaks existing user-space is by definition a kernel driver bug,
> regardless whether exisiting user-space is doing something horrendously
> stupid.

I wouldn't be so quick to say it's a kernel bug. The fbdev contract
hasn't changed. Also xf86-video-vmware isn't using the fbdev driver,
and the fbdev driver code is obviously correct (see screenshots in
link submitted with initial patch).

> So the fix must IMO be a kernel driver fix. My initial guess is that
> once we set the bytes per line register, it might not be automatically
> updated when the screen width is changed, but the documentation is poor.
> I see if I can shed some light over this.

Having dumped all of the svga registers while hacking on vmwgfx, I
noticed that the BYTES_PER_LINE field is initially incorrectly set to
something way off. My initial reaction is that video-vmware doesn't
properly compute the bytes-per-line register, and therefore that it is
a video-vmware bug that has always existed.

I'm reproducing the problem and providing a fix for video-vmware as I
write this.

C

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Thomas Hellstrom]

On 06/30/2014 02:25 PM, Christopher Friedt wrote:
> On Mon, Jun 30, 2014 at 7:48 AM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> I don't think we can blame video-vmware for this. A kernel driver change
>> that breaks existing user-space is by definition a kernel driver bug,
>> regardless whether exisiting user-space is doing something horrendously
>> stupid.
> I wouldn't be so quick to say it's a kernel bug. The fbdev contract
> hasn't changed. Also xf86-video-vmware isn't using the fbdev driver,
> and the fbdev driver code is obviously correct (see screenshots in
> link submitted with initial patch).

As I said. A kernel change that breaks existing user-space is ALWAYS BY
DEFINITION a kernel bug.
kernel changes are NOT ALLOWED to break existing user-space. The only
exception I can see here is if someone uses
the old non-kms driver which is not intended to work if vmware fbdev is
loaded but that's not the case here, from what I can tell?
>
>> So the fix must IMO be a kernel driver fix. My initial guess is that
>> once we set the bytes per line register, it might not be automatically
>> updated when the screen width is changed, but the documentation is poor.
>> I see if I can shed some light over this.
> Having dumped all of the svga registers while hacking on vmwgfx, I
> noticed that the BYTES_PER_LINE field is initially incorrectly set to
> something way off. My initial reaction is that video-vmware doesn't
> properly compute the bytes-per-line register, and therefore that it is
> a video-vmware bug that has always existed.

xf86-video-vmware in kms mode uses the kernel driver to set these registers.
FWIW, the modesetting part of the kernel driver uses SVGA_REG_PITCHLOCK
instead of
SVGA_REG_BYTES_PER_LINE to set the pitch. That's probably where the
clash happens.

/Thomas


>
> I'm reproducing the problem and providing a fix for video-vmware as I
> write this.
>
> C

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

On Mon, Jun 30, 2014 at 8:39 AM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
> xf86-video-vmware in kms mode uses the kernel driver to set these registers.
> FWIW, the modesetting part of the kernel driver uses SVGA_REG_PITCHLOCK
> instead of
> SVGA_REG_BYTES_PER_LINE to set the pitch. That's probably where the
> clash happens.

That sounds a bit more accurate. Should kms and fbdev be setting both
registers then?

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Thomas Hellstrom]

On 06/30/2014 02:49 PM, Christopher Friedt wrote:
> On Mon, Jun 30, 2014 at 8:39 AM, Thomas Hellstrom <thellstrom@vmware.com> wrote:
>> xf86-video-vmware in kms mode uses the kernel driver to set these registers.
>> FWIW, the modesetting part of the kernel driver uses SVGA_REG_PITCHLOCK
>> instead of
>> SVGA_REG_BYTES_PER_LINE to set the pitch. That's probably where the
>> clash happens.
> That sounds a bit more accurate. Should kms and fbdev be setting both
> registers then?

>From a quick browse of the device code, it looks like
SVGA_REG_BYTES_PER_LINE is intended for reading only, but I've asked the
experts that are on US time.

However, since the function
vmw_kms_write_svga()
should set up the pitch correctly using PITCHLOCK, could you just try
reverting the last line of your patch, the one that says

vmw_write(vmw_priv, SVGA_REG_BYTES_PER_LINE, info->fix.line_length);

That should hopefully fix the new bug and leave fbdev working as you
intended.

Thanks,
Thomas

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

On Mon, Jun 30, 2014 at 8:49 AM, Christopher Friedt
<chrisfriedt@gmail.com> wrote:
> That sounds a bit more accurate. Should kms and fbdev be setting both
> registers then?

I wonder if fbdev can use PITCHLOCK as well, rather than
BYTES_PER_LINE. I will only be able to run both kms and fbdev
functional tests about 9 hours from now, so any discussion until then
is welcome.

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Thomas Hellstrom]

On 06/30/2014 03:18 PM, Christopher Friedt wrote:
> On Mon, Jun 30, 2014 at 8:49 AM, Christopher Friedt
> <chrisfriedt@gmail.com> wrote:
>> That sounds a bit more accurate. Should kms and fbdev be setting both
>> registers then?
> I wonder if fbdev can use PITCHLOCK as well, rather than
> BYTES_PER_LINE. I will only be able to run both kms and fbdev
> functional tests about 9 hours from now, so any discussion until then
> is welcome

Please see the previous message I sent. If you just remove the line that
sets BYTES_PER_LINE,
fbdev will have called into the kms code to set PITCHLOCK correctly.

/Thomas

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

I have been unable to reproduce this issue in a plethora of test
cases, although granted, I do not have access to a Win7 machine. For
that, I have asked an Ubuntu tester to run through some tests for me.

https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-vmware/+bug/1328898/comments/8

Will post updates as I receive them.

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Thomas Hellstrom]

On 07/02/2014 05:01 AM, Christopher Friedt wrote:
> I have been unable to reproduce this issue in a plethora of test
> cases, although granted, I do not have access to a Win7 machine. For
> that, I have asked an Ubuntu tester to run through some tests for me.
>
> https://urldefense.proofpoint.com/v1/url?u=https://bugs.launchpad.net/ubuntu/%2Bsource/xserver-xorg-video-vmware/%2Bbug/1328898/comments/8&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=dUdnjNSVc8rpaH6Jc63zkOFO0Bari6xxf72ZYyMo%2Bk4%3D%0A&s=0c96076761f2340d2de78dcf53d1f2a616d7b932cae49bccbf8268f7520815c5
>
> Will post updates as I receive them
Indeed, same here, although I only tested on Linux host.

I've got confirmation from the device developers that the register in
question is indeed a read only register, so I'll post a patch.
However, it would be good if you could confirm that the fbdev problem
your patch fixed works also without the write to
SVGA_REG_BUTES_PER_LINE

Thanks,
Thomas

Re: [PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

At least one source has reported that vmwgfx_fb.c::vmw_fb_check_var()
is not even a code path that is traversed when the bug occurs (i.e.
inserted unique log message -> never seen in logs).

Also, everyone encountering the bug seems to be on a slightly older
version of VMWare Player. It does not seem to occur with "modern"
versions of VMWare Player, and might even be limited to Windows hosts.

So I think at this point, I can conclusively say It Wasn't Me™.
However, I am a bit concerned with this bug, so I'm going to follow up
on it within the Ubuntu community and help out in any way that I can.

In any case... I do have a pretty cool patch that enables (something
like) double-buffering on vmwgfx_fb - care to see it!!??

Hehehe... I'm serious! :-)

Cheers,

C

[PATCH 1/1] drm/vmwgfx: correct fb_fix_screeninfo.line_length

[Christopher Friedt]

Previously, the vmwgfx_fb driver would allow users to call FBIOSET_VINFO, but it would not adjust
the FINFO properly, resulting in distorted screen rendering. The patch corrects that behaviour.

See https://bugs.gentoo.org/show_bug.cgi?id=494794 for examples.

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index ed5ce2a..021b522 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -147,7 +147,7 @@ static int vmw_fb_check_var(struct fb_var_screeninfo *var,
 	}
 
 	if (!vmw_kms_validate_mode_vram(vmw_priv,
-					info->fix.line_length,
+					var->xres * var->bits_per_pixel/8,
 					var->yoffset + var->yres)) {
 		DRM_ERROR("Requested geom can not fit in framebuffer\n");
 		return -EINVAL;
@@ -162,6 +162,8 @@ static int vmw_fb_set_par(struct fb_info *info)
 	struct vmw_private *vmw_priv = par->vmw_priv;
 	int ret;
 
+	info->fix.line_length = info->var.xres * info->var.bits_per_pixel/8;
+
 	ret = vmw_kms_write_svga(vmw_priv, info->var.xres, info->var.yres,
 				 info->fix.line_length,
 				 par->bpp, par->depth);
@@ -177,6 +179,7 @@ static int vmw_fb_set_par(struct fb_info *info)
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_POSITION_Y, info->var.yoffset);
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_WIDTH, info->var.xres);
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_HEIGHT, info->var.yres);
+		vmw_write(vmw_priv, SVGA_REG_BYTES_PER_LINE, info->fix.line_length);
 		vmw_write(vmw_priv, SVGA_REG_DISPLAY_ID, SVGA_ID_INVALID);
 	}
 
-- 
1.8.3.2