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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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