Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n()

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

On Wed, Oct 16, 2019 at 11:02:03AM +0000, Mihail Atanassov wrote:
> On Wednesday, 16 October 2019 11:34:08 BST james qian wang (Arm Technology China) wrote:
> > Add a new helper function drm_color_ctm_s31_32_to_qm_n() for driver to
> > convert S31.32 sign-magnitude to Qm.n 2's complement that supported by
> > hardware.
> > 
> > V4: Address Mihai, Daniel and Ilia's review comments.
> > V5: Includes the sign bit in the value of m (Qm.n).
> > 
> > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++
> >  include/drm/drm_color_mgmt.h     |  2 ++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > index 4ce5c6d8de99..d313f194f1ec 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -132,6 +132,33 @@ uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision)
> >  }
> >  EXPORT_SYMBOL(drm_color_lut_extract);
> >  
> > +/**
> > + * drm_color_ctm_s31_32_to_qm_n
> > + *
> > + * @user_input: input value
> > + * @m: number of integer bits, include the sign-bit, support range is [1, 32]
> 
> Any reason why numbers like Q0.32 are disallowed? In those cases, the
> 'sign' bit and the first fractional bit just happen to be the same bit.
> The longer I look at it, the more I think mentioning a 'sign-bit' here
> might confuse people more, since 2's complement doesn't have a
> dedicated bit just for the sign. How about reducing it simply to:

No, since the value is signed there must be dedicated sign-bit.

consider very simple 2 bit signed, Q1.1

 0.5  is 01
 0    is 00
-0.5  is 11
-1.0  is 10, sign-bit and value share same bit, but it is integer part.

See the wiki:

One convention includes the sign bit in the value of m,[1][2] and the other convention
does not. The choice of convention can be determined by summing m+n. If the value is equal
to the register size, then the sign bit is included in the value of m. If it is one
less than the register size, the sign bit is not included in the value of m.

So for the 32bit value, all fractional:

a) M include sign-bit: Q1.31
b) M doesn't include sign-bit: Q0.31

> 
>  * @m: number of integer bits, m <= 32.
> 
> > + * @n: number of fractional bits, only support n <= 32
> > + *
> > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's complement). The
> > + * higher bits that above m + n are cleared or equal to sign-bit BIT(m+n).
> 
> [nit] BIT(m + n - 1) if we count from 0.

do we real need to consider this, convert to (Q1.0) :)
I think it can be easily caught by review.
> 
> > + */
> > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > +				      uint32_t m, uint32_t n)
> > +{
> > +	u64 mag = (user_input & ~BIT_ULL(63)) >> (32 - n);
> > +	bool negative = !!(user_input & BIT_ULL(63));
> > +	s64 val;
> > +
> > +	WARN_ON(m < 1 || m > 32 || n > 32);
> > +
> > +	/* the range of signed 2's complement is [-2^(m-1), 2^(m-1) - 2^-n] */
> > +	val = clamp_val(mag, 0, negative ?
> > +				BIT_ULL(n + m - 1) : BIT_ULL(n + m - 1) - 1);
> > +
> > +	return negative ? -val : val;
> > +}
> > +EXPORT_SYMBOL(drm_color_ctm_s31_32_to_qm_n);
> > +
> >  /**
> >   * drm_crtc_enable_color_mgmt - enable color management properties
> >   * @crtc: DRM CRTC
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index d1c662d92ab7..60fea5501886 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -30,6 +30,8 @@ struct drm_crtc;
> >  struct drm_plane;
> >  
> >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input,
> > +				      uint32_t m, uint32_t n);
> >  
> >  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >  				uint degamma_lut_size,
> > 
> 
> 
> -- 
> Mihail
> 
>