[PATCH] drm/i915/pch: Fix integer math bugs in panel fitting

[PATCH] drm/i915/pch: Fix integer math bugs in panel fitting

[Adam Jackson]

Consider a 1600x900 panel, upscaling a 1360x768 mode, full-aspect.  The
old math would give you:

    scaled_width  = 1600 * 768;         /* 1228800 */
    scaled_height = 1360 * 900;         /* 1224000 */
    if (scaled_width > scaled_height) { /* pillarbox, and true */
        width  = 1224000 / 768;         /* int(1593.75) = 1593 */
        x      = (1600 - 1593 + 1) / 2; /* 4 */
        y      = 0;
        height = 768;
    } /* ... */

This is broken.  The total width of scanout would then be 1593 + 4 + 4,
or 1601, which is wider than the panel itself.  The hardware very
dutifully implements this, and you end up with a black 45° diagonal from
the top-left corner to the bottom edge of the screen.  It's a cool
effect and all, but not what you wanted.  Similar things happen for the
letterbox case.

The problem is that you have an integer number of pixels, which means
it's usually impossible to upscale equally on both axes.  1360/768 is
1.7708, 1600/900 is 1.7777.  Since we're constrained on the one axis,
the other one wants to come out as an even number of pixels (the panel
is almost certainly even on both axes, and the x/y offsets will be
applied on both sides).  In the math above, if 'width' comes out even,
rounding down is correct; if it's odd, you'd rather round up.  So just
increment width/height in those cases.

Tested on a Lenovo T500 (Ironlake).

Signed-off-by: Adam Jackson <ajax@redhat.com>
---
 drivers/gpu/drm/i915/intel_panel.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index a06ff07..05f500c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -83,11 +83,15 @@ intel_pch_panel_fitting(struct drm_device *dev,
 			u32 scaled_height = mode->hdisplay * adjusted_mode->vdisplay;
 			if (scaled_width > scaled_height) { /* pillar */
 				width = scaled_height / mode->vdisplay;
+				if (width & 1)
+				    	width++;
 				x = (adjusted_mode->hdisplay - width + 1) / 2;
 				y = 0;
 				height = adjusted_mode->vdisplay;
 			} else if (scaled_width < scaled_height) { /* letter */
 				height = scaled_width / mode->hdisplay;
+				if (height & 1)
+				    height++;
 				y = (adjusted_mode->vdisplay - height + 1) / 2;
 				x = 0;
 				width = adjusted_mode->hdisplay;
-- 
1.7.6

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/pch: Fix integer math bugs in panel fitting

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 329 bytes --]

On Wed, 13 Jul 2011 16:32:32 -0400, Adam Jackson <ajax@redhat.com> wrote:

> +				if (width & 1)
> +				    	width++;


> +				if (height & 1)
> +				    height++;

You'll want to stick a comment in here describing why this is
here. Otherwise, someone will just break it in the future.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/pch: Fix integer math bugs in panel fitting

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 2753 bytes --]

On Wed, 2011-07-13 at 16:32 -0400, Adam Jackson wrote:
> Consider a 1600x900 panel, upscaling a 1360x768 mode, full-aspect.  The
> old math would give you:
> 
>     scaled_width  = 1600 * 768;         /* 1228800 */
>     scaled_height = 1360 * 900;         /* 1224000 */
>     if (scaled_width > scaled_height) { /* pillarbox, and true */
>         width  = 1224000 / 768;         /* int(1593.75) = 1593 */
>         x      = (1600 - 1593 + 1) / 2; /* 4 */
>         y      = 0;
>         height = 768;
>     } /* ... */
> 
> This is broken.  The total width of scanout would then be 1593 + 4 + 4,
> or 1601, which is wider than the panel itself.  The hardware very
> dutifully implements this, and you end up with a black 45° diagonal from
> the top-left corner to the bottom edge of the screen.  It's a cool
> effect and all, but not what you wanted.  Similar things happen for the
> letterbox case.
> 
> The problem is that you have an integer number of pixels, which means
> it's usually impossible to upscale equally on both axes.  1360/768 is
> 1.7708, 1600/900 is 1.7777.  Since we're constrained on the one axis,
> the other one wants to come out as an even number of pixels (the panel
> is almost certainly even on both axes, and the x/y offsets will be
> applied on both sides).  In the math above, if 'width' comes out even,
> rounding down is correct; if it's odd, you'd rather round up.  So just
> increment width/height in those cases.
> 
> Tested on a Lenovo T500 (Ironlake).
> 
> Signed-off-by: Adam Jackson <ajax@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index a06ff07..05f500c 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -83,11 +83,15 @@ intel_pch_panel_fitting(struct drm_device *dev,
>  			u32 scaled_height = mode->hdisplay * adjusted_mode->vdisplay;
>  			if (scaled_width > scaled_height) { /* pillar */
>  				width = scaled_height / mode->vdisplay;
> +				if (width & 1)
> +				    	width++;
>  				x = (adjusted_mode->hdisplay - width + 1) / 2;
>  				y = 0;
>  				height = adjusted_mode->vdisplay;
>  			} else if (scaled_width < scaled_height) { /* letter */
>  				height = scaled_width / mode->hdisplay;
> +				if (height & 1)
> +				    height++;
>  				y = (adjusted_mode->vdisplay - height + 1) / 2;
>  				x = 0;
>  				width = adjusted_mode->hdisplay;

We've seen this in Ubuntu in the form of
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/753994 so we can
add a

Tested-By: Daniel Manrique <daniel.manrique@canonical.com>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/pch: Fix integer math bugs in panel fitting

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]

On Wed, 13 Jul 2011 16:32:32 -0400, Adam Jackson <ajax@redhat.com> wrote:
> Consider a 1600x900 panel, upscaling a 1360x768 mode, full-aspect.  The
> old math would give you:
> 
>     scaled_width  = 1600 * 768;         /* 1228800 */
>     scaled_height = 1360 * 900;         /* 1224000 */
>     if (scaled_width > scaled_height) { /* pillarbox, and true */
>         width  = 1224000 / 768;         /* int(1593.75) = 1593 */
>         x      = (1600 - 1593 + 1) / 2; /* 4 */
>         y      = 0;
>         height = 768;
>     } /* ... */
> 
> This is broken.  The total width of scanout would then be 1593 + 4 + 4,
> or 1601, which is wider than the panel itself.  The hardware very
> dutifully implements this, and you end up with a black 45° diagonal from
> the top-left corner to the bottom edge of the screen.  It's a cool
> effect and all, but not what you wanted.  Similar things happen for the
> letterbox case.

I'm going to show my ignorance but why did the hw apply the offset to both
sides of the output and generate a new mode line? That's probably inherent
in the nature of the hblanks, I guess? The entirety of the description I
have is that the position is simply the origin of the top-left pixel of the
fitted window.

So a comment here is necessary, but otherwise:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38851
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

And a big thank you.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx