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

From: Daniel Vetter <daniel@ffwll.ch>
To: "james qian wang (Arm Technology China)" <james.qian.wang@arm.com>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>,
	"airlied@linux.ie" <airlied@linux.ie>,
	Brian Starkey <Brian.Starkey@arm.com>,
	"maarten.lankhorst@linux.intel.com" 
	<maarten.lankhorst@linux.intel.com>,
	"sean@poorly.run" <sean@poorly.run>,
	"imirkin@alum.mit.edu" <imirkin@alum.mit.edu>, nd <nd@arm.com>,
	Ayan Halder <Ayan.Halder@arm.com>,
	"Oscar Zhang (Arm Technology China)" <Oscar.Zhang@arm.com>,
	"Tiannan Zhu (Arm Technology China)" <Tiannan.Zhu@arm.com>,
	Mihail Atanassov <Mihail.Atanassov@arm.com>,
	"Jonathan Chai (Arm Technology China)" <Jonathan.Chai@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Julien Yin (Arm Technology China)" <Julien.Yin@arm.com>,
	"Channing Chen (Arm Technology China)" <Channing.Chen@arm.com>,
	"Yiqi Kang (Arm Technology China)" <Yiqi.Kang@arm.com>,
	"Thomas Sun (Arm Technology China)" <thomas.Sun@arm.com>,
	"Lowry Li (Arm Technology China)" <Lowry.Li@arm.com>,
	Ben Davis <Ben.Davis@arm.com>
Subject: Re: [PATCH v2 1/4] drm/komeda: Add a new helper drm_color_ctm_s31_32_to_qm_n()
Date: Mon, 14 Oct 2019 17:33:27 +0200	[thread overview]
Message-ID: <20191014153327.GL11828@phenom.ffwll.local> (raw)
In-Reply-To: <20191014095813.GA15227@jamwan02-TSP300>

On Mon, Oct 14, 2019 at 09:58:20AM +0000, james qian wang (Arm Technology China) wrote:
> On Mon, Oct 14, 2019 at 10:56:05AM +0200, Daniel Vetter wrote:
> > On Fri, Oct 11, 2019 at 05:43:09AM +0000, 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.
> > >
> > > Signed-off-by: james qian wang (Arm Technology China) <james.qian.wang@arm.com>
> > > ---
> > >  drivers/gpu/drm/drm_color_mgmt.c | 23 +++++++++++++++++++++++
> > >  include/drm/drm_color_mgmt.h     |  2 ++
> > >  2 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> > > index 4ce5c6d8de99..3d533d0b45af 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -132,6 +132,29 @@ 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
> > > + * @n: number of fractinal bits
> > > + *
> > > + * Convert and clamp S31.32 sign-magnitude to Qm.n 2's complement.
> >
> > What's the Q meaning here? Also maybe specify that the higher bits above
> > m+n are cleared to all 0 or all 1. Unit test would be lovely too. Anyway:
> 
> The Q used to represent signed two's complement.
> 
> For detail: https://en.wikipedia.org/wiki/Q_(number_format)
> 
> And it Q is 2's complement, so the value of higher bit equal to
> sign-bit.
> All 1 if it is negative
> 0 if it is positive.

Ah I didn't know about this notation, I think in other drm docs we just
used m.n 2's complement to denote this layout. Up to you I think.
-Daniel

> 
> James
> 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > > + */
> > > +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;
> > > +
> > > +   /* the range of signed 2s complement is [-2^n+m, 2^n+m - 1] */
> > > +   val = clamp_val(mag, 0, negative ? BIT(n + m) : BIT(n + m) - 1);
> > > +
> > > +   return negative ? 0ll - 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,
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch