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

[Mihail Atanassov]

On Friday, 18 October 2019 08:51:09 BST james qian wang (Arm Technology China) wrote:
> 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@xxxxxxx>
> > > Reviewed-by: Mihail Atanassov <mihail.atanassov@xxxxxxx>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > ---
> > >  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.

As I've said a few times before in this review, 2's complement has no
dedicated sign bit, that's the whole reason 2's complement exists in
the first place. The sign is implemented by negating the weight of
the most significant bit. This isn't a dedicated +/- field!

> 
> 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.

And a very simple 2-bit signed Q0.2 would look like this:

weights: (-2^-1)*b1 + (2^-2)*b0
          ^
          L-> note negative weight at most significant bit position

+-------------+---------------+
/ bit pattern | decimal value |
+-------------+---------------+
\     00      |     0.0       |
/     01      |     0.25      |
\     10      |    -0.5       |
/     11      |    -0.25      |
+-------------+---------------+

(Apologies for the ragged left border on the table :/)

I genuinely don't see why you really want to have that integer part be
strictly non-zero in size, it can very well be all fractional.

> 
> 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.

This is very much off the mark. See above for my sign bit in 2's
complement rant. With that caveat, what you refer to as the sign
bit is simply the top bit. If m+n is 16, then what you refer to as
the sign bit is in bit position 15 with a weight of -2^(m-1). If
m+n is instead 13, then all that changes is that the bit with the
weight of -2^(m-1) is at position 12.

Most importantly, there is nothing special about m + n == regsize,
the lack of sign-extension aside.

What I think is the source of confusion is that when you expand, say,
Q4.4 into a Q8.8, you need to do sign extension, so bit position 7
in the original Q4.4 needs to be replicated into positions 12-15 in
addition to moving it to position 11 in the destination format. But
then those are no longer sign bits, the signedness is encoded in bit
15 as a weight of -2^7 :).

> 
> So for the 32bit value, all fractional:
> 
> a) M include sign-bit: Q1.31

This is a 32-bit number with range [-1, 1 - 2^-31] and precision 2^-31.
The weight of bit 31 is simply -2^0 (i.e. -1). This has 1 integer bit.

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

This is a 31-bit number with range [-0.5, 1 - 2^-31]. Same precision as
above but smaller range. This is all fractional but not a 32-bit value.
I think you're looking for Q0.32, which has almost the same range but
double the precision.

> 
> > 
> >  * @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.

Q1.0 has a range of [-1, 0] and precision of 1, but I don't get how
this is relevant. I was just referring to convention that bits get
counted from 0, so the most significant bit is simply at position
m + n - 1 and not m + n.
m + n in, say, Q16.16 would be bit 32, which is just past the valid
[0..31] bits.

I was hoping that by explicitly tagging the comment with '[nit]' would
help convey its low importance :).

> > 
> > > + */
> > > +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