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

[james qian wang (Arm Technology China)]

On Fri, Oct 18, 2019 at 09:30:11AM +0000, Mihail Atanassov wrote:
> 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 :).
>

Thank you very much.

finial I got it, will update the patch in the next version 

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