On Tue, Oct 25, 2016 at 05:02:15PM -0200, Paulo Zanoni wrote:
> Em Seg, 2016-10-24 às 19:13 +0300, ville.syrjala@xxxxxxxxxxxxxxx
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Pass the framebuffer size in .16 fixed point coordinates to
> > drm_rect_rotate() since that's what the source coordinates are as
> > well
> > at this stage. We used to do this part of the computation in integer
> > coordinates, but that got changed when moving the computation to
> > happen in the check phase of the operation. Unfortunately I forgot
> > to shift up the fb width and height appropriately.
> >
> > With the bogus size we ended up with some negative fb offset, which
> > when
> > added to the vma offset caused out scanout to start at an offset
> > earlier
> > than we inteded. Eg. when testing on my SKL I saw a row of incorrect
> > tiles at the top of my screen.
>
> I already mentioned this while reviewing another patch from another
> author, but let's throw the idea again: how about adding a specific
> 16.16 type in order to prevent these silent failures when mixing them
> with integers?
>
> struct (or union) int_fixed_16_16 {
> uint32_t number;
> }
>
> And them some nice macros for explicit casting to/from int.
>
> I see include/drm/fixed.h even has a 20_12 type...
>
> I could even volunteer to do this if there's some positive feedback
> upfront, but feel free to do this too in case you want.
>
> We're humans and we're going to make the same "mix normal integers with
> 16.16 integers" mistake again and again and again, while the compiler
> could really help us if the types were not plain integers.
>
> Thoughts?
One ugliness is that we have struct drm_rect in 16_16 format, and others
as real integers. It would be fairly invasive I fear (touching lots of
drivers). But might be worth it. One intermediate step might be to just
add typedefs, since those can be casted silently. Still leaves the door
wide open for bugs, but at least it would serve as pretty good hints in
the code about what's going on.
Once we have the typedefs we could have the functions for rounding to
integers or converting integers to fixed point (same for drm_rect). And
once we have that we could exchange the typedefs for the opaque structs
like the one above.
Lots of work indeed ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
