Hi,

On 20/05/16 09:35, Jyri Sarha wrote:
> Add gamma table support to DSS dispc.
> 
> DSS driver initializes the default gamma table at component bind time
> and holds a copy of all gamma tables in it's internal data structure.

"its"

> Each call to dispc_mgr_set_gamma() updates the internal table and
> triggers write the HW, if it is enabled. The tables are restored to HW

"... to the HW"?

> in PM resume callback. The tables at DSS side matches the HW tables in
> size and in number of significant bits per color component. The

Maybe "the tables in RAM match the tables in HW".

> dispc_mgr_set_gamma() converts the size of any given table to match
> the table size in HW.
> 
> dispc_mgr_gamma_size() gives HW gamma table size for the channel
> and returns 0 if gamma table is not supported by HW or DSS driver.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c        | 189 ++++++++++++++++++++++++++---
>  drivers/gpu/drm/omapdrm/dss/dispc.h        |   5 +
>  drivers/gpu/drm/omapdrm/dss/dss_features.c |   2 +
>  drivers/gpu/drm/omapdrm/dss/dss_features.h |   1 +
>  drivers/gpu/drm/omapdrm/dss/hdmi4.c        |   3 -
>  drivers/gpu/drm/omapdrm/dss/hdmi5.c        |   3 -
>  drivers/gpu/drm/omapdrm/dss/omapdss.h      |   5 +
>  7 files changed, 186 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index f83608b..87dde05 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -116,6 +116,7 @@ struct dispc_features {
>  };
>  
>  #define DISPC_MAX_NR_FIFOS 5
> +#define DISPC_MAX_CHANNEL_GAMMA 4
>  
>  static struct {
>  	struct platform_device *pdev;
> @@ -135,6 +136,8 @@ static struct {
>  	bool		ctx_valid;
>  	u32		ctx[DISPC_SZ_REGS / sizeof(u32)];
>  
> +	u32 *gamma_table[DISPC_MAX_CHANNEL_GAMMA];
> +
>  	const struct dispc_features *feat;
>  
>  	bool is_enabled;
> @@ -178,11 +181,19 @@ struct dispc_reg_field {
>  	u8 low;
>  };
>  
> +struct dispc_gamma_desc {
> +	u32 len;
> +	u32 bits;
> +	u16 reg;
> +	bool has_index;
> +};
> +
>  static const struct {
>  	const char *name;
>  	u32 vsync_irq;
>  	u32 framedone_irq;
>  	u32 sync_lost_irq;
> +	struct dispc_gamma_desc gamma;
>  	struct dispc_reg_field reg_desc[DISPC_MGR_FLD_NUM];
>  } mgr_desc[] = {
>  	[OMAP_DSS_CHANNEL_LCD] = {
> @@ -190,6 +201,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_VSYNC,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONE,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST,
> +		.gamma		= {
> +			.len	= 256,
> +			.bits	= 8,
> +			.reg	= DISPC_GAMMA_TABLE0,
> +			.has_index = true,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL,  0,  0 },
>  			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL,  3,  3 },
> @@ -207,6 +224,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONETV,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST_DIGIT,
> +		.gamma		= {
> +			.len	= 1024,
> +			.bits	= 10,
> +			.reg	= DISPC_GAMMA_TABLE2,
> +			.has_index = false,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL,  1,  1 },
>  			[DISPC_MGR_FLD_STNTFT]		= { },
> @@ -224,6 +247,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_VSYNC2,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONE2,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST2,
> +		.gamma		= {
> +			.len	= 256,
> +			.bits	= 8,
> +			.reg	= DISPC_GAMMA_TABLE1,
> +			.has_index = true,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL2,  0,  0 },
>  			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL2,  3,  3 },
> @@ -241,6 +270,12 @@ static const struct {
>  		.vsync_irq	= DISPC_IRQ_VSYNC3,
>  		.framedone_irq	= DISPC_IRQ_FRAMEDONE3,
>  		.sync_lost_irq	= DISPC_IRQ_SYNC_LOST3,
> +		.gamma		= {
> +			.len	= 256,
> +			.bits	= 8,
> +			.reg	= DISPC_GAMMA_TABLE3,
> +			.has_index = true,
> +		},
>  		.reg_desc	= {
>  			[DISPC_MGR_FLD_ENABLE]		= { DISPC_CONTROL3,  0,  0 },
>  			[DISPC_MGR_FLD_STNTFT]		= { DISPC_CONTROL3,  3,  3 },
> @@ -1084,20 +1119,6 @@ static u32 dispc_ovl_get_burst_size(enum omap_plane plane)
>  	return unit * 8;
>  }
>  
> -void dispc_enable_gamma_table(bool enable)
> -{
> -	/*
> -	 * This is partially implemented to support only disabling of
> -	 * the gamma table.
> -	 */
> -	if (enable) {
> -		DSSWARN("Gamma table enabling for TV not yet supported");
> -		return;
> -	}
> -
> -	REG_FLD_MOD(DISPC_CONFIG, enable, 9, 9);
> -}
> -
>  static void dispc_mgr_enable_cpr(enum omap_channel channel, bool enable)
>  {
>  	if (channel == OMAP_DSS_CHANNEL_DIGIT)
> @@ -3814,6 +3835,128 @@ void dispc_disable_sidle(void)
>  	REG_FLD_MOD(DISPC_SYSCONFIG, 1, 4, 3);	/* SIDLEMODE: no idle */
>  }
>  
> +u32 dispc_mgr_gamma_size(enum omap_channel channel)
> +{
> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return 0;
> +

This should probably check the availability of LCD2/LCD3?

> +	return gdesc->len;
> +}
> +EXPORT_SYMBOL(dispc_mgr_gamma_size);
> +
> +static void dispc_mgr_write_gamma_table(enum omap_channel channel)
> +{
> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +	u32 *table = dispc.gamma_table[channel];
> +	unsigned int i;
> +
> +	DSSDBG("%s: channel %d\n", __func__, channel);
> +
> +	for (i = 0; i < gdesc->len; ++i) {
> +		u32 v = table[i];
> +
> +		if (gdesc->has_index)
> +			v |= i << 24;
> +		else if (i == 0)
> +			v |= 1 << 31;
> +
> +		dispc_write_reg(gdesc->reg, v);
> +	}
> +}
> +
> +static void dispc_restore_gamma_tables(void)
> +{
> +	DSSDBG("%s()\n", __func__);
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return;
> +
> +	dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD);
> +
> +	dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_DIGIT);
> +
> +	if (dss_has_feature(FEAT_MGR_LCD2))
> +		dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD2);
> +
> +	if (dss_has_feature(FEAT_MGR_LCD3))
> +		dispc_mgr_write_gamma_table(OMAP_DSS_CHANNEL_LCD3);
> +}
> +
> +void dispc_mgr_set_gamma(enum omap_channel channel,
> +			 const struct drm_color_lut *lut,
> +			 unsigned int length)
> +{
> +	const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +	u32 *table = dispc.gamma_table[channel];
> +	int i;

unsigned.

> +
> +	DSSDBG("%s: channel %d, lut len %u, hw len %u\n", __func__,
> +	       channel, length, gdesc->len);
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return;
> +
> +	for (i = 0; i < length; ++i) {
> +		int first = i * (gdesc->len - 1) / (length - 1);
> +		int last = (i + 1) * (gdesc->len - 1) / (length - 1);

This might be slightly more readable if you hade src_len and dst_len
local variables. Or maybe it's clear enough, it's a short function.

> +		int w = last - first;

unsigned.

> +		u16 r, g, b;
> +		int j;

unsigned.

> +
> +		for (j = 0; j <= w; j++) {
> +			r = (lut[i].red * (w - j) + lut[i+1].red * j) / w;
> +			g = (lut[i].green * (w - j) + lut[i+1].green * j) / w;
> +			b = (lut[i].blue * (w - j) + lut[i+1].blue * j) / w;
> +
> +			r >>= (16 - gdesc->bits);
> +			g >>= (16 - gdesc->bits);
> +			b >>= (16 - gdesc->bits);

I don't think the parenthesis do anything here.

> +
> +			table[first + j] = (r << (gdesc->bits * 2)) |
> +				(g << gdesc->bits) | b;
> +		}
> +	}
> +
> +	if (dispc.is_enabled)
> +		dispc_mgr_write_gamma_table(channel);
> +}
> +EXPORT_SYMBOL(dispc_mgr_set_gamma);
> +
> +static int dispc_init_gamma_tables(void)
> +{
> +	int channel;
> +
> +	if (!dss_has_feature(FEAT_GAMMA_TABLE))
> +		return;
> +
> +	for (channel = 0; channel < ARRAY_SIZE(dispc.gamma_table); channel++) {
> +		const struct dispc_gamma_desc *gdesc = &mgr_desc[channel].gamma;
> +		u32 *table;
> +		int i;

unsigned.

> +
> +		if (channel == OMAP_DSS_CHANNEL_LCD2 &&
> +		    !dss_has_feature(FEAT_MGR_LCD2))
> +			continue;
> +
> +		if (channel == OMAP_DSS_CHANNEL_LCD3 &&
> +		    !dss_has_feature(FEAT_MGR_LCD3))
> +			continue;
> +
> +		table = devm_kmalloc_array(&dispc.pdev->dev, gdesc->len,
> +					   sizeof(table[i]), GFP_KERNEL);

Wouldn't allocating gdesc->len * sizeof(u32) be much more
understandable? 'i' is even uninitialized at this point, making the code
quite suspicious.

> +		if (!table)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < gdesc->len; ++i)
> +			table[i] =
> +				(i << 2*gdesc->bits) | (i << gdesc->bits) | i;

Add an empty line here.

Add spaces around '*'.

And instead of splitting the line above, perhaps assign the value to u32
local, and assign that to table[i].

> +		dispc.gamma_table[channel] = table;
> +	}
> +	return 0;
> +}
> +
>  static void _omap_dispc_initial_config(void)
>  {
>  	u32 l;
> @@ -3829,8 +3972,16 @@ static void _omap_dispc_initial_config(void)
>  		dispc.core_clk_rate = dispc_fclk_rate();
>  	}
>  
> -	/* FUNCGATED */
> -	if (dss_has_feature(FEAT_FUNCGATED))
> +	/* Use gamma table */

Use gamma table for what, instead of what?

> +	if (dss_has_feature(FEAT_GAMMA_TABLE))
> +		REG_FLD_MOD(DISPC_CONFIG, 1, 3, 3);
> +
> +	/* For older DSS versions (FEAT_FUNCGATED) this enables
> +	 * func-clock auto-gating. For newer versions
> +	 * (FEAT_GAMMA_TABLE) this enables tv-out gamma tables.
> +	 */
> +	if (dss_has_feature(FEAT_FUNCGATED) ||
> +	    dss_has_feature(FEAT_GAMMA_TABLE))
>  		REG_FLD_MOD(DISPC_CONFIG, 1, 9, 9);
>  
>  	dispc_setup_color_conv_coef();
> @@ -4100,6 +4251,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
>  		}
>  	}
>  
> +	r = dispc_init_gamma_tables();
> +	if (r)
> +		return r;
> +
>  	pm_runtime_enable(&pdev->dev);
>  
>  	r = dispc_runtime_get();
> @@ -4170,6 +4325,8 @@ static int dispc_runtime_resume(struct device *dev)
>  		_omap_dispc_initial_config();
>  
>  		dispc_restore_context();
> +
> +		dispc_restore_gamma_tables();
>  	}
>  
>  	dispc.is_enabled = true;
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.h b/drivers/gpu/drm/omapdrm/dss/dispc.h
> index 4837442..bc1d812 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.h
> @@ -42,6 +42,11 @@
>  #define DISPC_MSTANDBY_CTRL		0x0858
>  #define DISPC_GLOBAL_MFLAG_ATTRIBUTE	0x085C
>  
> +#define DISPC_GAMMA_TABLE0		0x0630
> +#define DISPC_GAMMA_TABLE1		0x0634
> +#define DISPC_GAMMA_TABLE2		0x0638
> +#define DISPC_GAMMA_TABLE3		0x0850
> +
>  /* DISPC overlay registers */
>  #define DISPC_OVL_BA0(n)		(DISPC_OVL_BASE(n) + \
>  					DISPC_BA0_OFFSET(n))
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c
> index c886a29..f77b1c5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss_features.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c
> @@ -593,6 +593,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = {
>  	FEAT_ALPHA_FREE_ZORDER,
>  	FEAT_FIFO_MERGE,
>  	FEAT_BURST_2D,
> +	FEAT_GAMMA_TABLE,
>  };

The dss_features is deprecated. New features should be added to the
respective driver. In this case, dispc.c. See struct dispc_features.

 Tomi