| Message ID | 20210901175431.14060-1-alyssa@rosenzweig.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm: Add drm_fixed_16_16 helper | expand |
Hi Alyssa, Thank you for the patch. On Wed, Sep 01, 2021 at 01:54:27PM -0400, Alyssa Rosenzweig wrote: > This constructs a fixed 16.16 rational, useful to specify the minimum > and maximum scaling in drm_atomic_helper_check_plane_state. It is > open-coded as a macro in multiple drivers, so let's share the helper. > > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> > --- > include/drm/drm_fixed.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h > index 553210c02ee0..df1f369b4918 100644 > --- a/include/drm/drm_fixed.h > +++ b/include/drm/drm_fixed.h > @@ -208,4 +208,9 @@ static inline s64 drm_fixp_exp(s64 x) > return sum; > } > Missing documentation :-) > +static inline int drm_fixed_16_16(s32 mult, s32 div) You should return a s32. The function name isn't very explicit, and departs from the naming scheme of other functions in the same file. As fixed-point numbers are stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The function should probably be named drm_fixp_s16_16 from_fraction() then, but then the same logic should possibly be replicated to ensure optimal precision. I wonder if it wouldn't be best to simply use drm_fixp_from_fraction() and shift the result right by 16 bits. > +{ > + return (mult << 16) / div; > +} > + > #endif
> Missing documentation :-) Ack. > > +static inline int drm_fixed_16_16(s32 mult, s32 div) > > You should return a s32. Ack. > The function name isn't very explicit, and departs from the naming > scheme of other functions in the same file. As fixed-point numbers are > stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the > drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The > function should probably be named drm_fixp_s16_16 from_fraction() then, > but then the same logic should possibly be replicated to ensure optimal > precision. I wonder if it wouldn't be best to simply use > drm_fixp_from_fraction() and shift the result right by 16 bits. Sure, I'm not attached to the naming ... will wait to hear what colours everyone else wants the bikehed painted. As for the implementation, I just went with what was used across multiple drivers already (no chance of regressions that way) but could reuse other helpers if it's better..? If the behaviour changes this goes from a trivial cleanup to a much more invasive changeset. I don't own half of the hardware here.
On Wed, Sep 01, 2021 at 09:35:40PM -0400, Alyssa Rosenzweig wrote: > > Missing documentation :-) > > Ack. > > > > +static inline int drm_fixed_16_16(s32 mult, s32 div) > > > > You should return a s32. > > Ack. > > > The function name isn't very explicit, and departs from the naming > > scheme of other functions in the same file. As fixed-point numbers are > > stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the > > drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The > > function should probably be named drm_fixp_s16_16 from_fraction() then, > > but then the same logic should possibly be replicated to ensure optimal > > precision. I wonder if it wouldn't be best to simply use > > drm_fixp_from_fraction() and shift the result right by 16 bits. > > Sure, I'm not attached to the naming ... will wait to hear what colours > everyone else wants the bikehed painted. > > As for the implementation, I just went with what was used across > multiple drivers already (no chance of regressions that way) but could > reuse other helpers if it's better..? If the behaviour changes this goes > from a trivial cleanup to a much more invasive changeset. I don't own > half of the hardware here. But except for msm which may need testing, all other drivers use the existing FRAC_16_16() macro with constant arguments, so it shouldn't be difficult to ensure that the new function gives the same results.
diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h index 553210c02ee0..df1f369b4918 100644 --- a/include/drm/drm_fixed.h +++ b/include/drm/drm_fixed.h @@ -208,4 +208,9 @@ static inline s64 drm_fixp_exp(s64 x) return sum; } +static inline int drm_fixed_16_16(s32 mult, s32 div) +{ + return (mult << 16) / div; +} + #endif
This constructs a fixed 16.16 rational, useful to specify the minimum and maximum scaling in drm_atomic_helper_check_plane_state. It is open-coded as a macro in multiple drivers, so let's share the helper. Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> --- include/drm/drm_fixed.h | 5 +++++ 1 file changed, 5 insertions(+)