Message-ID: <1628395.4cazpm7HuF@e123338-lin>
Date:   Mon, 19 Aug 2019 10:45:53 +0000
From:   Mihail Atanassov <Mihail.Atanassov@....com>
To:     "james qian wang (Arm Technology China)" <james.qian.wang@....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>,
        "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>
Subject: Re: [PATCH v2 2/4] drm/komeda: Introduce komeda_color_manager/state

On Monday, 19 August 2019 09:50:55 BST james qian wang (Arm Technology China) wrote:
> On Wed, Aug 14, 2019 at 10:47:40AM +0000, Mihail Atanassov wrote:
> > On Wednesday, 14 August 2019 08:52:18 BST james qian wang (Arm Technology China) wrote:
> > > On Tue, Aug 13, 2019 at 09:51:08AM +0000, Mihail Atanassov wrote:
> > > > Hi James,
> > > > 
> > > > On Tuesday, 13 August 2019 05:56:07 BST james qian wang (Arm Technology China) wrote:
> > > > > Many komeda component support color management like layer and IPS, so
> > > > > komeda_color_manager/state are introduced to manager gamma, csc and degamma
> > > > > together for easily share it to multiple componpent.
> > > > > 
> > > > > And for komeda_color_manager which:
> > > > > - convert drm 3d gamma lut to komeda specific gamma coeffs
> > > > > - gamma table management and hide the HW difference for komeda-CORE
> > > > > 
> > > > > Signed-off-by: James Qian Wang (Arm Technology China) <james.qian.wang@....com>
> > > > > ---
> > > > >  .../arm/display/komeda/komeda_color_mgmt.c    | 126 ++++++++++++++++++
> > > > >  .../arm/display/komeda/komeda_color_mgmt.h    |  32 ++++-
> > > > >  2 files changed, 156 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > > index 9d14a92dbb17..bf2388d641b9 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.c
> > > > > @@ -4,7 +4,9 @@
> > > > >   * Author: James.Qian.Wang <james.qian.wang@....com>
> > > > >   *
> > > > >   */
> > > > > +#include <drm/drm_print.h>
> > > > >  
> > > > > +#include "malidp_utils.h"
> > > > >  #include "komeda_color_mgmt.h"
> > > > >  
> > > > >  /* 10bit precision YUV2RGB matrix */
> > > > > @@ -65,3 +67,127 @@ const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range)
> > > > >  
> > > > >  	return coeffs;
> > > > >  }
> > > > > +
> > > > > +struct gamma_curve_sector {
> > > > > +	u32 boundary_start;
> > > > > +	u32 num_of_segments;
> > > > > +	u32 segment_width;
> > > > > +};
> > > > > +
> > > > > +struct gamma_curve_segment {
> > > > > +	u32 start;
> > > > > +	u32 end;
> > > > > +};
> > > > > +
> > > > > +static struct gamma_curve_sector sector_tbl[] = {
> > > > > +	{ 0,    4,  4   },
> > Max LUT precision (see full response below) is determined by your
> > smallest segment, which is 4.
> > > > > +	{ 16,   4,  4   },
> > > > > +	{ 32,   4,  8   },
> > > > > +	{ 64,   4,  16  },
> > > > > +	{ 128,  4,  32  },
> > > > > +	{ 256,  4,  64  },
> > > > > +	{ 512,  16, 32  },
> > > > > +	{ 1024, 24, 128 },
> > > > > +};
> > > > > +
> > > > > +static struct gamma_curve_sector igamma_sector_tbl[] = {
> > > > > +	{0, 64, 64},
> > > > > +};
> > > > > +
> > > > > +static void
> > > > > +drm_lut_to_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs,
> > > > > +		  struct gamma_curve_sector *sector_tbl, u32 num_sectors)
> > > > > +{
> > > > > +	struct drm_color_lut *lut;
> > > > > +	u32 i, j, in, num = 0;
> > > > > +
> > > > > +	if (!lut_blob)
> > > > > +		return;
You need to validate the LUTs size here, or you risk reading past its end below.
Consider drm_color_lut_size(const struct drm_property_blob*).
> > > > > +
> > > > > +	lut = lut_blob->data;
> > > > > +
> > > > > +	for (i = 0; i < num_sectors; i++) {
> > > > > +		for (j = 0; j < sector_tbl[i].num_of_segments; j++) {
> > > > > +			in = sector_tbl[i].boundary_start +
> > > > > +			     j * sector_tbl[i].segment_width;
> > > > > +
> > > > > +			coeffs[num++] = drm_color_lut_extract(lut[in].red,
> > > > > +						KOMEDA_COLOR_PRECISION);
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	coeffs[num] = BIT(KOMEDA_COLOR_PRECISION);
> > > > > +}
> > > > > +
> > > > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > > > +{
> > > > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > > > +			  igamma_sector_tbl, ARRAY_SIZE(igamma_sector_tbl));
> > > > > +}
> > > > > +
> > > > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs)
> > > > > +{
> > > > > +	drm_lut_to_coeffs(lut_blob, coeffs,
> > > > > +			  sector_tbl, ARRAY_SIZE(sector_tbl));
> > > > > +}
> > > > > +
> > > > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs)
> > > > > +{
> > > > > +	struct drm_color_ctm *ctm;
> > > > > +	u32 i;
> > > > > +
> > > > > +	if (!ctm_blob)
> > > > > +		return;
> > > > > +
> > > > > +	ctm = ctm_blob->data;
> > > > > +
> > > > > +	for (i = 0; i < KOMEDA_N_CTM_COEFFS; ++i) {
> > > > > +		/* Convert from S31.32 to Q3.12. */
> > > > > +		s64 v = ctm->matrix[i];
> > > > > +
> > > > > +		coeffs[i] = clamp_val(v, 1 - (1LL << 34), (1LL << 34) - 1) >> 20;
> > > > CTM matrix values are S31.32, i.e. sign-magnitude, so clamp_val won't
> > > > give you the right result for negative coeffs. See
> > > > malidp_crtc_atomic_check_ctm for the sign-mag -> 2's complement
> > > > conversion.
> > > 
> > > Thank you Mihail for pointing this out.
> > > 
> > > No matter our user or kernel all assume this s31.32 as 2's complement. 
> > > we need to correct them both.
> > > 
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > > > +				  struct komeda_color_state *old)
> > > > [bikeshed] not really a _duplicate_state if all it does is refcounts.
> > > > kmemdup here and return a pointer (with ERR_PTR on fail), or memcpy if
> > > > you want to keep the signature?
> > > 
> > > Yes, the dup mostly should return a new ptr from a old, the dup name here
> > > is not accurate.
> > > the reason is the color_state is not a separated structure but always
> > > embedded into layer_state, but I want to make all color_state operation
> > > into a func.
> > > Do you have any suggestion ?
> > > 
> > After looking at the follow-up patch, not really (at least not any
> > good ones). I did tag it with [bikeshed] after all, it's not that
> > big a deal.
> > 
> > > > > +{
> > > > > +	new->igamma = komeda_coeffs_get(old->igamma);
> > > > > +	new->fgamma = komeda_coeffs_get(old->fgamma);
> > > > > +}
> > > > > +
> > > > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st)
> > > > > +{
> > > > > +	komeda_coeffs_put(color_st->igamma);
> > > > > +	komeda_coeffs_put(color_st->fgamma);
> > > > > +}
> > > > > +
> > > > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > > > +			  struct komeda_color_state *st,
> > > > > +			  struct drm_property_blob *igamma_blob,
> > > > > +			  struct drm_property_blob *fgamma_blob)
> > > > > +{
> > > > > +	u32 coeffs[KOMEDA_N_GAMMA_COEFFS];
> > > > > +
> > > > > +	komeda_color_cleanup_state(st);
> > > > > +
> > > > > +	if (igamma_blob) {
> > > > > +		drm_lut_to_igamma_coeffs(igamma_blob, coeffs);
> > > > > +		st->igamma = komeda_coeffs_request(mgr->igamma_mgr, coeffs);
> > > > > +		if (!st->igamma) {
> > > > > +			DRM_DEBUG_ATOMIC("request igamma table failed.\n");
> > > > > +			return -EBUSY;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (fgamma_blob) {
> > > > > +		drm_lut_to_fgamma_coeffs(fgamma_blob, coeffs);
> > > > > +		st->fgamma = komeda_coeffs_request(mgr->fgamma_mgr, coeffs);
> > > > > +		if (!st->fgamma) {
> > > > > +			DRM_DEBUG_ATOMIC("request fgamma table failed.\n");
> > > > > +			return -EBUSY;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > > index a2df218f58e7..41a96b3b540f 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_color_mgmt.h
> > > > > @@ -4,14 +4,42 @@
> > > > >   * Author: James.Qian.Wang <james.qian.wang@....com>
> > > > >   *
> > > > >   */
> > > > > -
> > > > >  #ifndef _KOMEDA_COLOR_MGMT_H_
> > > > >  #define _KOMEDA_COLOR_MGMT_H_
> > > > >  
> > > > >  #include <drm/drm_color_mgmt.h>
> > > > > +#include "komeda_coeffs.h"
> > > > >  
> > > > >  #define KOMEDA_N_YUV2RGB_COEFFS		12
> > > > > +#define KOMEDA_N_RGB2YUV_COEFFS		12
> > > > > +#define KOMEDA_COLOR_PRECISION		12
> > > > > +#define KOMEDA_N_GAMMA_COEFFS		65
> > > > > +#define KOMEDA_COLOR_LUT_SIZE		BIT(KOMEDA_COLOR_PRECISION)
> > > 
> > > > I don't see how the number of LUT entries has anything to do with the
> > > > bit-precision of each entry.
> > > 
> > > Because our komeda color is 12-bit precison, and for 1 vs 1 loopup
> > > table, we need BIT(12) entries.
> > > 
> > > Thank you
> > > James
> > > 
> > But your maximum possible precision in HW is 4 times less. You only
> > really need one LUT entry per segment (its start) in order to
> > define it (and the slope, but you get the idea). I.e. at your current
> > 4K-sized LUT table, the conversion to the internal representation only
> > _really_ needs to access offsets 0, 4, etc. and even less often as
> > it goes. If you make your table 1K entries instead, you save yourself
> > 24KiB every time the (i)gamma changes.
> > 
> > TL;DR: you don't need 1:1 lookup, you need a lossless conversion from
> > the LUT to the HW format.
> 
> Hi Mihail:
> 
> Thank you for raising this topic.
> 
> I had consider this before, but I dropped it finally. because the
> "compatibility".
> 
> Once we drop the 1vs1 lookup, but use a 1k table according the our D71
> which made this lut d71 specific and leads:
> - hard to compatable with third part user.
> - hard to compatable with the future HW.
> 
> And all these color_mgmt properties are DRM standard, we also need to
> follow DRM's way but not make it our HW only.
> 
> I don't see DRM directly say this table should be a 1 vs 1 lookup, but
> we can got some hint from the doc of drm_crtc property "DEGAMMA_LUT”: 
> 
> "Hardware might choose not to use the full precision of the LUT elements
> nor use all the elements of the LUT (for example the hardware might
> choose to interpolate between LUT[0] and LUT[4])"
> 
Won't help us if future hardware suddenly needs an 8K LUT ;).
Regardless, fixing all of that is easy with a u8 log2_lut_size in
komeda_dev when the time comes, which is why I raised this now.
There's no point planning for what we don't know how it'll turn out
given that the current solution may become inadequate either way.

Now, all that said, the GAMMA_LUT_SIZE prop has an interesting sentence
in its description:
"""
 *         If drivers support multiple LUT sizes then they should publish the
 *	largest size, and sub-sample smaller sized LUTs (e.g. for split-gamma
 *	modes) appropriately.
"""

I'll back down on this and say exposing 4K LUTs is fine, and if we
need to speed up redoing the LUTs, userland can send a smaller LUT
(or maybe even a pre-baked coeffs table if we're allowed).

Looking at i915's use of the props, the code accepts multiple different
sizes and interpretations of what is in the LUT (e.g. a 256 pallette
for C8) depending the HW, with >1 accepted sizes for a
particular HW. Given that there's precedent and we can enhance this
code later if needed (e.g. to save 24KiB per table as per my original
concern), please fix the LUT size check above and you have my:

Reviewed-by: Mihail Atanassov <mihail.atanassov@....com>

> thanks
> james.
> 

> > > > > +#define KOMEDA_N_CTM_COEFFS		9
> > > > > +
> > > > > +struct komeda_color_manager {
> > > > > +	struct komeda_coeffs_manager *igamma_mgr;
> > > > > +	struct komeda_coeffs_manager *fgamma_mgr;
> > > > > +	bool has_ctm;
> > > > > +};
> > > > > +
> > > > > +struct komeda_color_state {
> > > > > +	struct komeda_coeffs_table *igamma;
> > > > > +	struct komeda_coeffs_table *fgamma;
> > > > > +};
> > > > > +
> > > > > +void komeda_color_duplicate_state(struct komeda_color_state *new,
> > > > > +				  struct komeda_color_state *old);
> > > > > +void komeda_color_cleanup_state(struct komeda_color_state *color_st);
> > > > > +int komeda_color_validate(struct komeda_color_manager *mgr,
> > > > > +			  struct komeda_color_state *st,
> > > > > +			  struct drm_property_blob *igamma_blob,
> > > > > +			  struct drm_property_blob *fgamma_blob);
> > > > > +
> > > > > +void drm_lut_to_igamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > > > +void drm_lut_to_fgamma_coeffs(struct drm_property_blob *lut_blob, u32 *coeffs);
> > > > > +void drm_ctm_to_coeffs(struct drm_property_blob *ctm_blob, u32 *coeffs);
> > > > >  
> > > > >  const s32 *komeda_select_yuv2rgb_coeffs(u32 color_encoding, u32 color_range);
> > > > >  
> > > > > -#endif
> > > > > +#endif /*_KOMEDA_COLOR_MGMT_H_*/
> > > > > 
> > > > 
> > > > BR,
> > > > Mihail
> > > > 
> > > > 
> > > 
> > 
> > BR,
> > Mihail
> > 
> > 
> 

BR,
Mihail

Powered by blists - more mailing lists