| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191018101606.GA26967@jamwan02-TSP300>
Date: Fri, 18 Oct 2019 10:16:13 +0000
From: "james qian wang (Arm Technology China)" <james.qian.wang@....com>
To: Mihail Atanassov <Mihail.Atanassov@....com>
CC: Liviu Dudau <Liviu.Dudau@....com>,
"airlied@...ux.ie" <airlied@...ux.ie>,
Brian Starkey <Brian.Starkey@....com>,
"maarten.lankhorst@...ux.intel.com"
<maarten.lankhorst@...ux.intel.com>,
"sean@...rly.run" <sean@...rly.run>,
"imirkin@...m.mit.edu" <imirkin@...m.mit.edu>,
"Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
"Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
"Thomas Sun (Arm Technology China)" <thomas.Sun@....com>,
"Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
Ayan Halder <Ayan.Halder@....com>,
"Tiannan Zhu (Arm Technology China)" <Tiannan.Zhu@....com>,
"Yiqi Kang (Arm Technology China)" <Yiqi.Kang@....com>,
nd <nd@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
Ben Davis <Ben.Davis@....com>,
"Oscar Zhang (Arm Technology China)" <Oscar.Zhang@....com>,
"Channing Chen (Arm Technology China)" <Channing.Chen@....com>,
Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH v5 1/4] drm: Add a new helper
drm_color_ctm_s31_32_to_qm_n()
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@....com>
> > > > Reviewed-by: Mihail Atanassov <mihail.atanassov@....com>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@...ll.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.
>
> 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
>
>
Powered by blists - more mailing lists