[PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

[PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's try to make sure the fb offset computations never hit
an integer overflow by making sure the entire fb stays
below 32bits. framebuffer_check() in the core already does
the same check, but as it doesn't know about tiling some things
can slip through. Repeat the check in the driver with tiling
taken into account.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e642b7717106..67259c719ffe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2400,10 +2400,26 @@ static int intel_fb_offset_to_xy(int *x, int *y,
 				 int color_plane)
 {
 	struct drm_i915_private *dev_priv = to_i915(fb->dev);
+	unsigned int height;
 
 	if (fb->modifier != DRM_FORMAT_MOD_LINEAR &&
-	    fb->offsets[color_plane] % intel_tile_size(dev_priv))
+	    fb->offsets[color_plane] % intel_tile_size(dev_priv)) {
+		DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n",
+			      fb->offsets[color_plane], color_plane);
 		return -EINVAL;
+	}
+
+	height = drm_framebuffer_plane_height(fb->height, fb, color_plane);
+	height = ALIGN(height, intel_tile_height(fb, color_plane));
+
+	/* Catch potential overflows early */
+	if (mul_u32_u32(height, fb->pitches[color_plane]) +
+	    fb->offsets[color_plane] > UINT_MAX) {
+		DRM_DEBUG_KMS("Bad offset 0x%08x or pitch %d for color plane %d\n",
+			      fb->offsets[color_plane], fb->pitches[color_plane],
+			      color_plane);
+		return -ERANGE;
+	}
 
 	*x = 0;
 	*y = 0;
-- 
2.16.4

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

✓ Fi.CI.BAT: success for drm/i915: Make sure fb gtt offsets stay within 32bits

[Patchwork]

== Series Details ==

Series: drm/i915: Make sure fb gtt offsets stay within 32bits
URL   : https://patchwork.freedesktop.org/series/49985/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4852 -> Patchwork_10243 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49985/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10243 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-bdw-samus:       NOTRUN -> INCOMPLETE (fdo#107773)

    igt@kms_psr@primary_page_flip:
      fi-cnl-u:           PASS -> FAIL (fdo#107336)

    igt@pm_rpm@module-reload:
      {fi-skl-caroline}:  NOTRUN -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-bdw-samus:       INCOMPLETE (fdo#107773) -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS +1

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#103191, fdo#107362) -> PASS

    igt@kms_psr@primary_page_flip:
      fi-whl-u:           FAIL (fdo#107336) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807


== Participating hosts (52 -> 48) ==

  Additional (1): fi-skl-caroline 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4852 -> Patchwork_10243

  CI_DRM_4852: c7249769bf8b7da87c0f3d8e343a7c342f0f4c16 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4647: ae8187922d8de2bc739519da3bd40cf5f03f5e4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10243: 387bf87ad18338496b51e7b45585ba16245e8231 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

387bf87ad183 drm/i915: Make sure fb gtt offsets stay within 32bits

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10243/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

[Chris Wilson]

Quoting Ville Syrjala (2018-09-20 20:10:18)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Let's try to make sure the fb offset computations never hit
> an integer overflow by making sure the entire fb stays
> below 32bits. framebuffer_check() in the core already does
> the same check, but as it doesn't know about tiling some things
> can slip through. Repeat the check in the driver with tiling
> taken into account.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e642b7717106..67259c719ffe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2400,10 +2400,26 @@ static int intel_fb_offset_to_xy(int *x, int *y,
>                                  int color_plane)
>  {
>         struct drm_i915_private *dev_priv = to_i915(fb->dev);
> +       unsigned int height;
>  
>         if (fb->modifier != DRM_FORMAT_MOD_LINEAR &&
> -           fb->offsets[color_plane] % intel_tile_size(dev_priv))
> +           fb->offsets[color_plane] % intel_tile_size(dev_priv)) {
> +               DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n",
> +                             fb->offsets[color_plane], color_plane);
>                 return -EINVAL;
> +       }
> +
> +       height = drm_framebuffer_plane_height(fb->height, fb, color_plane);
> +       height = ALIGN(height, intel_tile_height(fb, color_plane));
> +
> +       /* Catch potential overflows early */
> +       if (mul_u32_u32(height, fb->pitches[color_plane]) +

if (add_overflows(mul_u32_u32(height, fb->pitches[color_plane]),
		  fb->offsets[color_plane],
		  U32_MAX) {
?

> +           fb->offsets[color_plane] > UINT_MAX) {
> +               DRM_DEBUG_KMS("Bad offset 0x%08x or pitch %d for color plane %d\n",
> +                             fb->offsets[color_plane], fb->pitches[color_plane],
> +                             color_plane);
> +               return -ERANGE;
> +       }
>  
>         *x = 0;
>         *y = 0;
> -- 
> 2.16.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Make sure fb gtt offsets stay within 32bits

[Patchwork]

== Series Details ==

Series: drm/i915: Make sure fb gtt offsets stay within 32bits
URL   : https://patchwork.freedesktop.org/series/49985/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4852_full -> Patchwork_10243_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_10243_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_eio@in-flight-1us:
      shard-glk:          PASS -> FAIL (fdo#105957)

    igt@kms_busy@extended-modeset-hang-newfb-render-c:
      shard-apl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_legacy@pipe-b-single-bo:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          NOTRUN -> FAIL (fdo#103166)

    
    ==== Possible fixes ====

    igt@gem_ctx_isolation@vcs0-s3:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@gem_exec_big:
      shard-hsw:          TIMEOUT (fdo#107937) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@gem_render_copy_redux@normal:
      shard-kbl:          INCOMPLETE (fdo#106650, fdo#103665) -> PASS

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-apl:          DMESG-WARN (fdo#107956) -> PASS +1

    igt@kms_chv_cursor_fail@pipe-b-128x128-left-edge:
      shard-kbl:          FAIL (fdo#104671) -> PASS

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-kbl:          FAIL (fdo#103232, fdo#103191) -> PASS
      shard-apl:          FAIL (fdo#103232, fdo#103191) -> PASS

    igt@kms_flip_tiling@flip-to-yf-tiled:
      shard-apl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +3
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@perf@blocking:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105957 https://bugs.freedesktop.org/show_bug.cgi?id=105957
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106650 https://bugs.freedesktop.org/show_bug.cgi?id=106650
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4852 -> Patchwork_10243

  CI_DRM_4852: c7249769bf8b7da87c0f3d8e343a7c342f0f4c16 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4647: ae8187922d8de2bc739519da3bd40cf5f03f5e4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10243: 387bf87ad18338496b51e7b45585ba16245e8231 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10243/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

[Ville Syrjälä]

On Thu, Sep 20, 2018 at 09:07:35PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-09-20 20:10:18)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Let's try to make sure the fb offset computations never hit
> > an integer overflow by making sure the entire fb stays
> > below 32bits. framebuffer_check() in the core already does
> > the same check, but as it doesn't know about tiling some things
> > can slip through. Repeat the check in the driver with tiling
> > taken into account.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e642b7717106..67259c719ffe 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2400,10 +2400,26 @@ static int intel_fb_offset_to_xy(int *x, int *y,
> >                                  int color_plane)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > +       unsigned int height;
> >  
> >         if (fb->modifier != DRM_FORMAT_MOD_LINEAR &&
> > -           fb->offsets[color_plane] % intel_tile_size(dev_priv))
> > +           fb->offsets[color_plane] % intel_tile_size(dev_priv)) {
> > +               DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n",
> > +                             fb->offsets[color_plane], color_plane);
> >                 return -EINVAL;
> > +       }
> > +
> > +       height = drm_framebuffer_plane_height(fb->height, fb, color_plane);
> > +       height = ALIGN(height, intel_tile_height(fb, color_plane));
> > +
> > +       /* Catch potential overflows early */
> > +       if (mul_u32_u32(height, fb->pitches[color_plane]) +
> 
> if (add_overflows(mul_u32_u32(height, fb->pitches[color_plane]),
> 		  fb->offsets[color_plane],
> 		  U32_MAX) {
> ?

Didn't know we had that. Looks like what we have right now doesn't quite
work as it only takes two arguments.

Oh interesting. __builtin_add_overflow_p() & co. work supposedly work on
bitfields as well.

Bah. Looks like this stuff generatess worse code than the normal thing:

    401e:       f7 e3                   mul    %ebx
    4020:       89 45 ac                mov    %eax,-0x54(%ebp)
    4023:       89 c8                   mov    %ecx,%eax
    4025:       89 55 b0                mov    %edx,-0x50(%ebp)
    4028:       31 d2                   xor    %edx,%edx
    402a:       03 45 ac                add    -0x54(%ebp),%eax
    402d:       13 55 b0                adc    -0x50(%ebp),%edx
    4030:       83 fa 00                cmp    $0x0,%edx
    4033:       0f 87 87 02 00 00       ja     42c0 <intel_framebuffer_init+0xb50>

vs.

    401a:       f7 65 b8                mull   -0x48(%ebp)
    401d:       89 45 ac                mov    %eax,-0x54(%ebp)
    4020:       89 d8                   mov    %ebx,%eax
    4022:       89 55 b0                mov    %edx,-0x50(%ebp)
    4025:       31 d2                   xor    %edx,%edx
    4027:       03 45 ac                add    -0x54(%ebp),%eax
    402a:       13 55 b0                adc    -0x50(%ebp),%edx
    402d:       3b 55 b0                cmp    -0x50(%ebp),%edx
    4030:       77 0c                   ja     403e <intel_framebuffer_init+0x8ce>
    4032:       72 05                   jb     4039 <intel_framebuffer_init+0x8c9>
    4034:       3b 45 ac                cmp    -0x54(%ebp),%eax
    4037:       73 05                   jae    403e <intel_framebuffer_init+0x8ce>
    4039:       b9 01 00 00 00          mov    $0x1,%ecx
    403e:       85 d2                   test   %edx,%edx
    4040:       0f 85 9a 02 00 00       jne    42e0 <intel_framebuffer_init+0xb70>
    4046:       85 c9                   test   %ecx,%ecx
    4048:       0f 85 92 02 00 00       jne    42e0 <intel_framebuffer_init+0xb70>

So five branches instead of just the one. That seems a bit excessive.

> 
> > +           fb->offsets[color_plane] > UINT_MAX) {
> > +               DRM_DEBUG_KMS("Bad offset 0x%08x or pitch %d for color plane %d\n",
> > +                             fb->offsets[color_plane], fb->pitches[color_plane],
> > +                             color_plane);
> > +               return -ERANGE;
> > +       }
> >  
> >         *x = 0;
> >         *y = 0;
> > -- 
> > 2.16.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

[Chris Wilson]

Quoting Ville Syrjälä (2018-09-21 14:06:02)
> On Thu, Sep 20, 2018 at 09:07:35PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-09-20 20:10:18)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Let's try to make sure the fb offset computations never hit
> > > an integer overflow by making sure the entire fb stays
> > > below 32bits. framebuffer_check() in the core already does
> > > the same check, but as it doesn't know about tiling some things
> > > can slip through. Repeat the check in the driver with tiling
> > > taken into account.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index e642b7717106..67259c719ffe 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2400,10 +2400,26 @@ static int intel_fb_offset_to_xy(int *x, int *y,
> > >                                  int color_plane)
> > >  {
> > >         struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > > +       unsigned int height;
> > >  
> > >         if (fb->modifier != DRM_FORMAT_MOD_LINEAR &&
> > > -           fb->offsets[color_plane] % intel_tile_size(dev_priv))
> > > +           fb->offsets[color_plane] % intel_tile_size(dev_priv)) {
> > > +               DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n",
> > > +                             fb->offsets[color_plane], color_plane);
> > >                 return -EINVAL;
> > > +       }
> > > +
> > > +       height = drm_framebuffer_plane_height(fb->height, fb, color_plane);
> > > +       height = ALIGN(height, intel_tile_height(fb, color_plane));
> > > +
> > > +       /* Catch potential overflows early */
> > > +       if (mul_u32_u32(height, fb->pitches[color_plane]) +
> > 
> > if (add_overflows(mul_u32_u32(height, fb->pitches[color_plane]),
> >                 fb->offsets[color_plane],
> >                 U32_MAX) {
> > ?
> 
> Didn't know we had that. Looks like what we have right now doesn't quite
> work as it only takes two arguments.

True, we would need the mul_overflows as well I guess (unless we are
happy that's eliminated earlier).
 
> Oh interesting. __builtin_add_overflow_p() & co. work supposedly work on
> bitfields as well.
> 
> Bah. Looks like this stuff generatess worse code than the normal thing:

True. Maybe harmless though since we are on an init path (and who
would reinitialise their fb on every update...) So the dilemma is having
if (add_overflows()) better than a /* check if add overflows */ comment,
and is that worth waiting on better code generation.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make sure fb gtt offsets stay within 32bits

[Ville Syrjälä]

On Fri, Sep 21, 2018 at 05:15:40PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-09-21 14:06:02)
> > On Thu, Sep 20, 2018 at 09:07:35PM +0100, Chris Wilson wrote:
> > > Quoting Ville Syrjala (2018-09-20 20:10:18)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Let's try to make sure the fb offset computations never hit
> > > > an integer overflow by making sure the entire fb stays
> > > > below 32bits. framebuffer_check() in the core already does
> > > > the same check, but as it doesn't know about tiling some things
> > > > can slip through. Repeat the check in the driver with tiling
> > > > taken into account.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 18 +++++++++++++++++-
> > > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index e642b7717106..67259c719ffe 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -2400,10 +2400,26 @@ static int intel_fb_offset_to_xy(int *x, int *y,
> > > >                                  int color_plane)
> > > >  {
> > > >         struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > > > +       unsigned int height;
> > > >  
> > > >         if (fb->modifier != DRM_FORMAT_MOD_LINEAR &&
> > > > -           fb->offsets[color_plane] % intel_tile_size(dev_priv))
> > > > +           fb->offsets[color_plane] % intel_tile_size(dev_priv)) {
> > > > +               DRM_DEBUG_KMS("Misaligned offset 0x%08x for color plane %d\n",
> > > > +                             fb->offsets[color_plane], color_plane);
> > > >                 return -EINVAL;
> > > > +       }
> > > > +
> > > > +       height = drm_framebuffer_plane_height(fb->height, fb, color_plane);
> > > > +       height = ALIGN(height, intel_tile_height(fb, color_plane));
> > > > +
> > > > +       /* Catch potential overflows early */
> > > > +       if (mul_u32_u32(height, fb->pitches[color_plane]) +
> > > 
> > > if (add_overflows(mul_u32_u32(height, fb->pitches[color_plane]),
> > >                 fb->offsets[color_plane],
> > >                 U32_MAX) {
> > > ?
> > 
> > Didn't know we had that. Looks like what we have right now doesn't quite
> > work as it only takes two arguments.
> 
> True, we would need the mul_overflows as well I guess (unless we are
> happy that's eliminated earlier).

I guess we'd really want a mac_overflows() to document what it's
really checking.

>  
> > Oh interesting. __builtin_add_overflow_p() & co. work supposedly work on
> > bitfields as well.
> > 
> > Bah. Looks like this stuff generatess worse code than the normal thing:
> 
> True. Maybe harmless though since we are on an init path (and who
> would reinitialise their fb on every update...) So the dilemma is having
> if (add_overflows()) better than a /* check if add overflows */ comment,
> and is that worth waiting on better code generation.

Yeah, in this case doesn't really matter. Just disappointed that
a builtin gives us suboptimal code even though the documentation
goes out of its way to point out that it can use fancy optimizations.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx