[Intel-gfx] [PATCH 18/23] drm/i915: Shrink the size of intel_remapped_plane_info struct

[Ville Syrjälä]

On Fri, Mar 12, 2021 at 12:19:42AM +0200, Imre Deak wrote:
> On Thu, Mar 11, 2021 at 09:45:14PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 11, 2021 at 12:17:31AM +0200, Imre Deak wrote:
> > > Save some place in the GTT VMAs by using a u16 instead of unsigned int
> > > to store the view dimensions. The maximum FB stride is 256kB which is
> > > 4096 tiles in the worst case (yf-tiles), the maximum FB height is 16k
> > > pixels, which is 2048 tiles in the worst case (x-tiles).
> > 
> > Actually I think the worst case for height is remapping linear fbs
> > since we more or less treat it as 4kx1 tiles. But 16k is still< 64k
> > so should be all good.
> 
> Yes, thanks for catching that. Will fix the commit message.
> 
> > Integer promotion stuff/etc. is what worried me the most here, but
> > looks like rotate_pages()/remap_pages() at least gets everything
> > passed in as unsigned int, so we're not in danger of sign bit
> > shenanigans there at least.
> 
> Yes. Fwiw I can think only of the following kind of sign-extension
> problem scenario with u16:
> 
> u16 v1=-1;
> int i=v1;
> 
> So if u16 stored a negative result, we'd miss the expected
> sign-extension, but I can't see a way we wanted to store a negative
> value to these fields. So for instance in remap_pages() the
> 
> 	offset += src_stride - width;
> 
> would be still correct even if src_stride/width would be u16 and
> src_stride was less than width.

I suppose most issues would be along the lines of expecting
u16-u16 to produce a mod u16 result. But since things get
promoted to signed int that is no longer true when the result
is negative. Also if you then cast that result to some bigger
type you can get even more sign extension happening.

-- 
Ville Syrjälä
Intel