From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH 2/4] drm/omapdrm: Add gamma table support to DSS dispc Date: Fri, 20 May 2016 15:51:52 +0300 Message-ID: <573F0868.8090700@ti.com> References: Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0813728096==" Return-path: Received: from arroyo.ext.ti.com (arroyo.ext.ti.com [198.47.19.12]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4901C6E366 for ; Fri, 20 May 2016 12:52:01 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jyri Sarha , dri-devel@lists.freedesktop.org Cc: peter.ujfalusi@ti.com, laurent.pinchart@ideasonboard.com List-Id: dri-devel@lists.freedesktop.org --===============0813728096== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OclcHM1v1Tdl3s46Qs6ux0al8vu7OHTI5" --OclcHM1v1Tdl3s46Qs6ux0al8vu7OHTI5 Content-Type: multipart/mixed; boundary="V1wo1TmHHddGCgAsvWpXo5Daat3QcALQM" From: Tomi Valkeinen To: Jyri Sarha , dri-devel@lists.freedesktop.org Cc: airlied@linux.ie, daniel@ffwll.ch, peter.ujfalusi@ti.com, bparrot@ti.com, laurent.pinchart@ideasonboard.com Message-ID: <573F0868.8090700@ti.com> Subject: Re: [PATCH 2/4] drm/omapdrm: Add gamma table support to DSS dispc References: In-Reply-To: --V1wo1TmHHddGCgAsvWpXo5Daat3QcALQM Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi, On 20/05/16 09:35, Jyri Sarha wrote: > Add gamma table support to DSS dispc. >=20 > 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. >=20 > 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. >=20 > Signed-off-by: Jyri Sarha > --- > 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(-) >=20 > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omap= drm/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 { > }; > =20 > #define DISPC_MAX_NR_FIFOS 5 > +#define DISPC_MAX_CHANNEL_GAMMA 4 > =20 > static struct { > struct platform_device *pdev; > @@ -135,6 +136,8 @@ static struct { > bool ctx_valid; > u32 ctx[DISPC_SZ_REGS / sizeof(u32)]; > =20 > + u32 *gamma_table[DISPC_MAX_CHANNEL_GAMMA]; > + > const struct dispc_features *feat; > =20 > bool is_enabled; > @@ -178,11 +181,19 @@ struct dispc_reg_field { > u8 low; > }; > =20 > +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[] =3D { > [OMAP_DSS_CHANNEL_LCD] =3D { > @@ -190,6 +201,12 @@ static const struct { > .vsync_irq =3D DISPC_IRQ_VSYNC, > .framedone_irq =3D DISPC_IRQ_FRAMEDONE, > .sync_lost_irq =3D DISPC_IRQ_SYNC_LOST, > + .gamma =3D { > + .len =3D 256, > + .bits =3D 8, > + .reg =3D DISPC_GAMMA_TABLE0, > + .has_index =3D true, > + }, > .reg_desc =3D { > [DISPC_MGR_FLD_ENABLE] =3D { DISPC_CONTROL, 0, 0 }, > [DISPC_MGR_FLD_STNTFT] =3D { DISPC_CONTROL, 3, 3 }, > @@ -207,6 +224,12 @@ static const struct { > .vsync_irq =3D DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN, > .framedone_irq =3D DISPC_IRQ_FRAMEDONETV, > .sync_lost_irq =3D DISPC_IRQ_SYNC_LOST_DIGIT, > + .gamma =3D { > + .len =3D 1024, > + .bits =3D 10, > + .reg =3D DISPC_GAMMA_TABLE2, > + .has_index =3D false, > + }, > .reg_desc =3D { > [DISPC_MGR_FLD_ENABLE] =3D { DISPC_CONTROL, 1, 1 }, > [DISPC_MGR_FLD_STNTFT] =3D { }, > @@ -224,6 +247,12 @@ static const struct { > .vsync_irq =3D DISPC_IRQ_VSYNC2, > .framedone_irq =3D DISPC_IRQ_FRAMEDONE2, > .sync_lost_irq =3D DISPC_IRQ_SYNC_LOST2, > + .gamma =3D { > + .len =3D 256, > + .bits =3D 8, > + .reg =3D DISPC_GAMMA_TABLE1, > + .has_index =3D true, > + }, > .reg_desc =3D { > [DISPC_MGR_FLD_ENABLE] =3D { DISPC_CONTROL2, 0, 0 }, > [DISPC_MGR_FLD_STNTFT] =3D { DISPC_CONTROL2, 3, 3 }, > @@ -241,6 +270,12 @@ static const struct { > .vsync_irq =3D DISPC_IRQ_VSYNC3, > .framedone_irq =3D DISPC_IRQ_FRAMEDONE3, > .sync_lost_irq =3D DISPC_IRQ_SYNC_LOST3, > + .gamma =3D { > + .len =3D 256, > + .bits =3D 8, > + .reg =3D DISPC_GAMMA_TABLE3, > + .has_index =3D true, > + }, > .reg_desc =3D { > [DISPC_MGR_FLD_ENABLE] =3D { DISPC_CONTROL3, 0, 0 }, > [DISPC_MGR_FLD_STNTFT] =3D { DISPC_CONTROL3, 3, 3 }, > @@ -1084,20 +1119,6 @@ static u32 dispc_ovl_get_burst_size(enum omap_pl= ane plane) > return unit * 8; > } > =20 > -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 enabl= e) > { > if (channel =3D=3D OMAP_DSS_CHANNEL_DIGIT) > @@ -3814,6 +3835,128 @@ void dispc_disable_sidle(void) > REG_FLD_MOD(DISPC_SYSCONFIG, 1, 4, 3); /* SIDLEMODE: no idle */ > } > =20 > +u32 dispc_mgr_gamma_size(enum omap_channel channel) > +{ > + const struct dispc_gamma_desc *gdesc =3D &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 =3D &mgr_desc[channel].gamma; > + u32 *table =3D dispc.gamma_table[channel]; > + unsigned int i; > + > + DSSDBG("%s: channel %d\n", __func__, channel); > + > + for (i =3D 0; i < gdesc->len; ++i) { > + u32 v =3D table[i]; > + > + if (gdesc->has_index) > + v |=3D i << 24; > + else if (i =3D=3D 0) > + v |=3D 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 =3D &mgr_desc[channel].gamma; > + u32 *table =3D 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 =3D 0; i < length; ++i) { > + int first =3D i * (gdesc->len - 1) / (length - 1); > + int last =3D (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 =3D last - first; unsigned. > + u16 r, g, b; > + int j; unsigned. > + > + for (j =3D 0; j <=3D w; j++) { > + r =3D (lut[i].red * (w - j) + lut[i+1].red * j) / w; > + g =3D (lut[i].green * (w - j) + lut[i+1].green * j) / w; > + b =3D (lut[i].blue * (w - j) + lut[i+1].blue * j) / w; > + > + r >>=3D (16 - gdesc->bits); > + g >>=3D (16 - gdesc->bits); > + b >>=3D (16 - gdesc->bits); I don't think the parenthesis do anything here. > + > + table[first + j] =3D (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 =3D 0; channel < ARRAY_SIZE(dispc.gamma_table); channel+= +) { > + const struct dispc_gamma_desc *gdesc =3D &mgr_desc[channel].gamma; > + u32 *table; > + int i; unsigned. > + > + if (channel =3D=3D OMAP_DSS_CHANNEL_LCD2 && > + !dss_has_feature(FEAT_MGR_LCD2)) > + continue; > + > + if (channel =3D=3D OMAP_DSS_CHANNEL_LCD3 && > + !dss_has_feature(FEAT_MGR_LCD3)) > + continue; > + > + table =3D 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 =3D 0; i < gdesc->len; ++i) > + table[i] =3D > + (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] =3D 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 =3D dispc_fclk_rate(); > } > =20 > - /* 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); > =20 > dispc_setup_color_conv_coef(); > @@ -4100,6 +4251,10 @@ static int dispc_bind(struct device *dev, struct= device *master, void *data) > } > } > =20 > + r =3D dispc_init_gamma_tables(); > + if (r) > + return r; > + > pm_runtime_enable(&pdev->dev); > =20 > r =3D dispc_runtime_get(); > @@ -4170,6 +4325,8 @@ static int dispc_runtime_resume(struct device *de= v) > _omap_dispc_initial_config(); > =20 > dispc_restore_context(); > + > + dispc_restore_gamma_tables(); > } > =20 > dispc.is_enabled =3D true; > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.h b/drivers/gpu/drm/omap= drm/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 > =20 > +#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/d= rm/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[]= =3D { > 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 --V1wo1TmHHddGCgAsvWpXo5Daat3QcALQM-- --OclcHM1v1Tdl3s46Qs6ux0al8vu7OHTI5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXPwhoAAoJEPo9qoy8lh71M5IP/3w+UCAhk4zeB5tAgCxx0xKG 4nd/7FsWZK0AK2HE+3cBCyJnMH9SBs8DfGMZx4wmOPVDJlx+D8VCFTcQpNe9CvUt H8n6wavrAmLS84GLRRXGgMPuTZGttb+U7tQ4XbMUVFK5pHuPD+WRtaOohmrhV8f0 eyMWzu5r2CmvXf4iIwPLY0uA+i6Wc9EJsi8aReccrovggPalWVmgaHRlcKbz2UUR AY/rXruLwYAizrMN62vbqTNXejH5NCPiLXCGAGwIvuXHn2Gm/ceuSdJecQQAOQ/m j/XnLAZ7cSV5zRM0ONy4UnyrhM/CDB57PZ6YhN9M0HaMPTriFiDRTovxpCVdvE5T +gx1g8jcSZbskEZ31UaApV+V3hXIDv9hK60KmxiN1ELCeu5LUdVZJ+iPRzxIKSab y2W83L3aI3hRDP58yjqF68XiniI5YArfEgdVxPYxgJKwz2UMWpYOThtAWLZUvzUz 9L29KzgdHwwdoBFMTs3SGGhpzLCauJoe3LyZYDjiGJW8gmLlvoBuFepCOOQj1Lpm LK5YN+TcnEAoAHvt9cD7SplWIUWmrGR7oCQftKiKgRu4yHsf9u9KNxFEs5UYWXGb uhE2m1h2ookUGDdGEiL/NYxlTOdcLHSjtZyrmEQjXMZ/TdtnVkyZa9Y4PL3IkOTU xwH6cTZOVLLkNZa8Vxb+ =AhUi -----END PGP SIGNATURE----- --OclcHM1v1Tdl3s46Qs6ux0al8vu7OHTI5-- --===============0813728096== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0813728096==--