From mboxrd@z Thu Jan 1 00:00:00 1970 From: "james qian wang (Arm Technology China)" Subject: Re: [PATCH v5 1/4] drm: Add a new helper drm_color_ctm_s31_32_to_qm_n() Date: Fri, 18 Oct 2019 10:16:13 +0000 Message-ID: <20191018101606.GA26967@jamwan02-TSP300> References: <20191016103339.25858-1-james.qian.wang@arm.com> <2404938.QDdPyV61sH@e123338-lin> <20191018075101.GA19928@jamwan02-TSP300> <4381055.oiViQHVQgJ@e123338-lin> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <4381055.oiViQHVQgJ@e123338-lin> Content-Language: en-US Content-ID: <2ACF7231D728B443810BFD099710F1A7@eurprd08.prod.outlook.com> Sender: linux-kernel-owner@vger.kernel.org To: Mihail Atanassov Cc: Liviu Dudau , "airlied@linux.ie" , Brian Starkey , "maarten.lankhorst@linux.intel.com" , "sean@poorly.run" , "imirkin@alum.mit.edu" , "Jonathan Chai (Arm Technology China)" , "Julien Yin (Arm Technology China)" , "Thomas Sun (Arm Technology China)" , "Lowry Li (Arm Technology China)" , Ayan Halder , "Tiannan Zhu (Arm Technology China)" , "Yiqi Kang (Arm Technology China)" , nd , "linux-kernel@vger.kernel.org" , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 C= hina) 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 Techn= ology 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. > > > >=20 > > > > V4: Address Mihai, Daniel and Ilia's review comments. > > > > V5: Includes the sign bit in the value of m (Qm.n). > > > >=20 > > > > Signed-off-by: james qian wang (Arm Technology China) > > > > Reviewed-by: Mihail Atanassov > > > > Reviewed-by: Daniel Vetter > > > > --- > > > > drivers/gpu/drm/drm_color_mgmt.c | 27 +++++++++++++++++++++++++++ > > > > include/drm/drm_color_mgmt.h | 2 ++ > > > > 2 files changed, 29 insertions(+) > > > >=20 > > > > 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_i= nput, uint32_t bit_precision) > > > > } > > > > EXPORT_SYMBOL(drm_color_lut_extract); > > > > =20 > > > > +/** > > > > + * 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] > > >=20 > > > 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 bi= t. > > > The longer I look at it, the more I think mentioning a 'sign-bit' her= e > > > might confuse people more, since 2's complement doesn't have a > > > dedicated bit just for the sign. How about reducing it simply to: > >=20 > > No, since the value is signed there must be dedicated sign-bit. >=20 > 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! >=20 > >=20 > > consider very simple 2 bit signed, Q1.1 > >=20 > > 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. >=20 > And a very simple 2-bit signed Q0.2 would look like this: >=20 > weights: (-2^-1)*b1 + (2^-2)*b0 > ^ > L-> note negative weight at most significant bit position >=20 > +-------------+---------------+ > / bit pattern | decimal value | > +-------------+---------------+ > \ 00 | 0.0 | > / 01 | 0.25 | > \ 10 | -0.5 | > / 11 | -0.25 | > +-------------+---------------+ >=20 > (Apologies for the ragged left border on the table :/) >=20 > 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. >=20 > >=20 > > See the wiki: > >=20 > > One convention includes the sign bit in the value of m,[1][2] and the o= ther 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. >=20 > 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. >=20 > Most importantly, there is nothing special about m + n =3D=3D regsize, > the lack of sign-extension aside. >=20 > 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=20 > >=20 > > So for the 32bit value, all fractional: > >=20 > > a) M include sign-bit: Q1.31 >=20 > 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. >=20 > > b) M doesn't include sign-bit: Q0.31 >=20 > 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. >=20 > >=20 > > >=20 > > > * @m: number of integer bits, m <=3D 32. > > >=20 > > > > + * @n: number of fractional bits, only support n <=3D 32 > > > > + * > > > > + * Convert and clamp S31.32 sign-magnitude to Qm.n (signed 2's com= plement). The > > > > + * higher bits that above m + n are cleared or equal to sign-bit B= IT(m+n). > > >=20 > > > [nit] BIT(m + n - 1) if we count from 0. > >=20 > > do we real need to consider this, convert to (Q1.0) :) > > I think it can be easily caught by review. >=20 > 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. >=20 > I was hoping that by explicitly tagging the comment with '[nit]' would > help convey its low importance :). >=20 > > >=20 > > > > + */ > > > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input, > > > > + uint32_t m, uint32_t n) > > > > +{ > > > > + u64 mag =3D (user_input & ~BIT_ULL(63)) >> (32 - n); > > > > + bool negative =3D !!(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 =3D 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_m= gmt.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; > > > > =20 > > > > uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_p= recision); > > > > +uint64_t drm_color_ctm_s31_32_to_qm_n(uint64_t user_input, > > > > + uint32_t m, uint32_t n); > > > > =20 > > > > void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > > > > uint degamma_lut_size, > > > >=20 > > >=20 > > >=20 > >=20 >=20 >=20 > --=20 > Mihail >=20 >=20