* [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
@ 2016-09-07 9:22 Meng Yi
2016-09-07 17:05 ` Stefan Agner
2016-09-13 7:02 ` Stefan Agner
0 siblings, 2 replies; 11+ messages in thread
From: Meng Yi @ 2016-09-07 9:22 UTC (permalink / raw)
To: stefan; +Cc: jianwei.wang.chn, Meng Yi, dri-devel
Gamma correction is optional and can be used to adjust the color
output values to match the gamut of a particular TFT LCD panel
Errata:
Gamma_R, Gamma_G and Gamma_B registers are little-endian registers
while the rest of the address-space in 2D-ACE is big-endian.
Workaround:
Split the DCU regs into "regs", "palette", "gamma" and "cursor".
Create a second regmap for gamma memory space using little endian.
Suggested-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Meng Yi <meng.yi@nxp.com>
---
Changes in V3:
-created a second regmap for gamma
-updated the DCU DT binding
---
.../devicetree/bindings/display/fsl,dcu.txt | 6 +++-
drivers/gpu/drm/fsl-dcu/Kconfig | 6 ++++
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 38 ++++++++++++++++++++++
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 30 ++++++++++++++++-
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 8 +++++
5 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt b/Documentation/devicetree/bindings/display/fsl,dcu.txt
index 63ec2a6..1b1321a 100644
--- a/Documentation/devicetree/bindings/display/fsl,dcu.txt
+++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt
@@ -20,7 +20,11 @@ Optional properties:
Examples:
dcu: dcu@2ce0000 {
compatible = "fsl,ls1021a-dcu";
- reg = <0x0 0x2ce0000 0x0 0x10000>;
+ reg = <0x0 0x2ce0000 0x0 0x2000>,
+ <0x0 0x2ce2000 0x0 0x2000>,
+ <0x0 0x2ce4000 0x0 0xc00>,
+ <0x0 0x2ce4c00 0x0 0x400>;
+ reg-names = "regs", "palette", "gamma", "cursor";
clocks = <&platform_clk 0>, <&platform_clk 0>;
clock-names = "dcu", "pix";
big-endian;
diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig
index 14a72c4..f9c76b1 100644
--- a/drivers/gpu/drm/fsl-dcu/Kconfig
+++ b/drivers/gpu/drm/fsl-dcu/Kconfig
@@ -11,3 +11,9 @@ config DRM_FSL_DCU
help
Choose this option if you have an Freescale DCU chipset.
If M is selected the module will be called fsl-dcu-drm.
+
+config DRM_FSL_DCU_GAMMA
+ bool "Gamma Correction Support for NXP/Freescale DCU"
+ depends on DRM_FSL_DCU
+ help
+ Enable support for gamma correction.
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
index 3371635..25ab0ff 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
@@ -22,6 +22,22 @@
#include "fsl_dcu_drm_drv.h"
#include "fsl_dcu_drm_plane.h"
+static void fsl_crtc_gamma_set(struct drm_crtc *crtc, struct drm_color_lut *lut,
+ uint32_t size)
+{
+ struct fsl_dcu_drm_device *fsl_dev = crtc->dev->dev_private;
+ unsigned int i;
+
+ for (i = 0; i < size; i++) {
+ regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_R + 4 * i,
+ lut[i].red);
+ regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_G + 4 * i,
+ lut[i].green);
+ regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_B + 4 * i,
+ lut[i].blue);
+ }
+}
+
static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
@@ -37,6 +53,11 @@ static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc,
drm_crtc_send_vblank_event(crtc, event);
spin_unlock_irq(&crtc->dev->event_lock);
}
+
+ if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
+ fsl_crtc_gamma_set(crtc, (struct drm_color_lut *)
+ crtc->state->gamma_lut->data,
+ 256);
}
static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
@@ -46,6 +67,10 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
drm_crtc_vblank_off(crtc);
+ if (fsl_dev->enable_color_mgmt)
+ regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+ DCU_MODE_EN_GAMMA_MASK, 0);
+
regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
DCU_MODE_DCU_MODE_MASK,
DCU_MODE_DCU_MODE(DCU_MODE_OFF));
@@ -58,6 +83,11 @@ static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc)
struct drm_device *dev = crtc->dev;
struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
+ if (fsl_dev->enable_color_mgmt)
+ regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
+ DCU_MODE_EN_GAMMA_MASK,
+ DCU_MODE_GAMMA_ENABLE);
+
regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
DCU_MODE_DCU_MODE_MASK,
DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
@@ -135,6 +165,7 @@ static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = {
.page_flip = drm_atomic_helper_page_flip,
.reset = drm_atomic_helper_crtc_reset,
.set_config = drm_atomic_helper_set_config,
+ .gamma_set = drm_atomic_helper_legacy_gamma_set,
};
int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
@@ -158,5 +189,12 @@ int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs);
+#ifdef CONFIG_DRM_FSL_DCU_GAMMA
+ fsl_dev->enable_color_mgmt = true;
+
+ drm_crtc_enable_color_mgmt(crtc, 0, false, 256);
+ drm_mode_crtc_set_gamma_size(crtc, 256);
+#endif
+
return 0;
}
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 7882387..f70ea68 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -48,6 +48,15 @@ static const struct regmap_config fsl_dcu_regmap_config = {
.volatile_reg = fsl_dcu_drm_is_volatile_reg,
};
+static const struct regmap_config fsl_dcu_regmap_gamma_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .val_format_endian = REGMAP_ENDIAN_LITTLE,
+
+ .volatile_reg = fsl_dcu_drm_is_volatile_reg,
+};
+
static int fsl_dcu_drm_irq_init(struct drm_device *dev)
{
struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
@@ -323,7 +332,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
struct drm_device *drm;
struct device *dev = &pdev->dev;
struct resource *res;
- void __iomem *base;
+ void __iomem *base, *base_gamma;
struct drm_driver *driver = &fsl_dcu_drm_driver;
struct clk *pix_clk_in;
char pix_clk_name[32];
@@ -365,6 +374,25 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
return PTR_ERR(fsl_dev->regmap);
}
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma");
+ if (!res) {
+ dev_err(dev, "could not get gamma memory resource\n");
+ return -ENODEV;
+ }
+
+ base_gamma = devm_ioremap_resource(dev, res);
+ if (IS_ERR(base_gamma)) {
+ ret = PTR_ERR(base_gamma);
+ return ret;
+ }
+
+ fsl_dev->regmap_gamma = devm_regmap_init_mmio(dev, base_gamma,
+ &fsl_dcu_regmap_config);
+ if (IS_ERR(fsl_dev->regmap_gamma)) {
+ dev_err(dev, "regmap_gamma init failed\n");
+ return PTR_ERR(fsl_dev->regmap_gamma);
+ }
+
fsl_dev->clk = devm_clk_get(dev, "dcu");
if (IS_ERR(fsl_dev->clk)) {
dev_err(dev, "failed to get dcu clock\n");
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
index 3b371fe7..95427f6 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -25,6 +25,8 @@
#define DCU_MODE_NORMAL 1
#define DCU_MODE_TEST 2
#define DCU_MODE_COLORBAR 3
+#define DCU_MODE_EN_GAMMA_MASK 0x04
+#define DCU_MODE_GAMMA_ENABLE BIT(2)
#define DCU_BGND 0x0014
#define DCU_BGND_R(x) ((x) << 16)
@@ -165,6 +167,10 @@
#define VF610_LAYER_REG_NUM 9
#define LS1021A_LAYER_REG_NUM 10
+#define FSL_GAMMA_R 0x000
+#define FSL_GAMMA_G 0x400
+#define FSL_GAMMA_B 0x800
+
struct clk;
struct device;
struct drm_device;
@@ -182,6 +188,7 @@ struct fsl_dcu_drm_device {
struct device *dev;
struct device_node *np;
struct regmap *regmap;
+ struct regmap *regmap_gamma;
int irq;
struct clk *clk;
struct clk *pix_clk;
@@ -195,6 +202,7 @@ struct fsl_dcu_drm_device {
struct fsl_dcu_drm_connector connector;
const struct fsl_dcu_soc_data *soc;
struct drm_atomic_state *state;
+ bool enable_color_mgmt;
};
void fsl_dcu_fbdev_init(struct drm_device *dev);
--
2.1.0.27.g96db324
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-07 9:22 [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties Meng Yi
@ 2016-09-07 17:05 ` Stefan Agner
2016-09-08 2:45 ` Meng Yi
2016-09-13 7:02 ` Stefan Agner
1 sibling, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-09-07 17:05 UTC (permalink / raw)
To: Meng Yi, robh+dt, frowand.list, broonie; +Cc: jianwei.wang.chn, dri-devel
On 2016-09-07 02:22, Meng Yi wrote:
> Gamma correction is optional and can be used to adjust the color
> output values to match the gamut of a particular TFT LCD panel
> Errata:
> Gamma_R, Gamma_G and Gamma_B registers are little-endian registers
> while the rest of the address-space in 2D-ACE is big-endian.
> Workaround:
> Split the DCU regs into "regs", "palette", "gamma" and "cursor".
> Create a second regmap for gamma memory space using little endian.
>
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Signed-off-by: Meng Yi <meng.yi@nxp.com>
> ---
> Changes in V3:
> -created a second regmap for gamma
> -updated the DCU DT binding
> ---
> .../devicetree/bindings/display/fsl,dcu.txt | 6 +++-
> drivers/gpu/drm/fsl-dcu/Kconfig | 6 ++++
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 38 ++++++++++++++++++++++
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 30 ++++++++++++++++-
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 8 +++++
> 5 files changed, 86 insertions(+), 2 deletions(-)
>
diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt
b/Documentation/devicetree/bindings/display/fsl,dcu.txt
index 63ec2a6..1b1321a 100644
--- a/Documentation/devicetree/bindings/display/fsl,dcu.txt
+++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt
@@ -20,7 +20,11 @@ Optional properties:
Examples:
dcu: dcu@2ce0000 {
compatible = "fsl,ls1021a-dcu";
- reg = <0x0 0x2ce0000 0x0 0x10000>;
+ reg = <0x0 0x2ce0000 0x0 0x2000>,
+ <0x0 0x2ce2000 0x0 0x2000>,
+ <0x0 0x2ce4000 0x0 0xc00>,
+ <0x0 0x2ce4c00 0x0 0x400>;
+ reg-names = "regs", "palette", "gamma", "cursor";
clocks = <&platform_clk 0>, <&platform_clk 0>;
clock-names = "dcu", "pix";
big-endian;
Please also extend the documentation of reg and reg-names above.
> diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> index 63ec2a6..1b1321a 100644
> --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> @@ -20,7 +20,11 @@ Optional properties:
> Examples:
> dcu: dcu@2ce0000 {
> compatible = "fsl,ls1021a-dcu";
> - reg = <0x0 0x2ce0000 0x0 0x10000>;
> + reg = <0x0 0x2ce0000 0x0 0x2000>,
> + <0x0 0x2ce2000 0x0 0x2000>,
> + <0x0 0x2ce4000 0x0 0xc00>,
> + <0x0 0x2ce4c00 0x0 0x400>;
> + reg-names = "regs", "palette", "gamma", "cursor";
> clocks = <&platform_clk 0>, <&platform_clk 0>;
> clock-names = "dcu", "pix";
> big-endian;
Looks good to me, I feel splitting up the register map makes sense
anyway. It is also documented that way:
36.4 Memory Map
Table 36-5. Memory map
Parameter Address Range
Register address space 0x0000 – 0x1FFF
Palette/Tile address space 0x2000 – 0x3FFF
Gamma_R address space 0x4000 – 0x43FF
Gamma_G address space 0x4400 – 0x47FF
Gamma_B address space 0x4800 – 0x4BFF
Cursor address space 0x4C00 – 0x4FFF
Can I have an Ack from the device tree maintainers here?
<snip>
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> @@ -22,6 +22,22 @@
> #include "fsl_dcu_drm_drv.h"
> #include "fsl_dcu_drm_plane.h"
>
> +static void fsl_crtc_gamma_set(struct drm_crtc *crtc, struct
> drm_color_lut *lut,
> + uint32_t size)
> +{
> + struct fsl_dcu_drm_device *fsl_dev = crtc->dev->dev_private;
> + unsigned int i;
> +
> + for (i = 0; i < size; i++) {
> + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_R + 4 * i,
> + lut[i].red);
> + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_G + 4 * i,
> + lut[i].green);
> + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_B + 4 * i,
> + lut[i].blue);
> + }
> +}
> +
> static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
<snip>
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -48,6 +48,15 @@ static const struct regmap_config fsl_dcu_regmap_config = {
> .volatile_reg = fsl_dcu_drm_is_volatile_reg,
> };
>
> +static const struct regmap_config fsl_dcu_regmap_gamma_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .val_format_endian = REGMAP_ENDIAN_LITTLE,
I would like to have a comment here why we force little endian.
> +
> + .volatile_reg = fsl_dcu_drm_is_volatile_reg,
> +};
> +
<snip>
> @@ -365,6 +374,25 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
> return PTR_ERR(fsl_dev->regmap);
> }
>
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma");
> + if (!res) {
> + dev_err(dev, "could not get gamma memory resource\n");
> + return -ENODEV;
> + }
> +
> + base_gamma = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base_gamma)) {
> + ret = PTR_ERR(base_gamma);
> + return ret;
> + }
> +
> + fsl_dev->regmap_gamma = devm_regmap_init_mmio(dev, base_gamma,
> + &fsl_dcu_regmap_config);
> + if (IS_ERR(fsl_dev->regmap_gamma)) {
> + dev_err(dev, "regmap_gamma init failed\n");
> + return PTR_ERR(fsl_dev->regmap_gamma);
> + }
> +
Mark, what do you think, is this a reasonable approach to work around
this errata?
--
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-07 17:05 ` Stefan Agner
@ 2016-09-08 2:45 ` Meng Yi
0 siblings, 0 replies; 11+ messages in thread
From: Meng Yi @ 2016-09-08 2:45 UTC (permalink / raw)
To: Stefan Agner, robh+dt, frowand.list, broonie; +Cc: jianwei.wang.chn, dri-devel
>
> > diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> > b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> > index 63ec2a6..1b1321a 100644
> > --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> > +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> > @@ -20,7 +20,11 @@ Optional properties:
> > Examples:
> > dcu: dcu@2ce0000 {
> > compatible = "fsl,ls1021a-dcu";
> > - reg = <0x0 0x2ce0000 0x0 0x10000>;
> > + reg = <0x0 0x2ce0000 0x0 0x2000>,
> > + <0x0 0x2ce2000 0x0 0x2000>,
> > + <0x0 0x2ce4000 0x0 0xc00>,
> > + <0x0 0x2ce4c00 0x0 0x400>;
> > + reg-names = "regs", "palette", "gamma", "cursor";
> > clocks = <&platform_clk 0>, <&platform_clk 0>;
> > clock-names = "dcu", "pix";
> > big-endian;
>
> Looks good to me, I feel splitting up the register map makes sense anyway. It is
> also documented that way:
>
> 36.4 Memory Map
> Table 36-5. Memory map
>
> Parameter Address Range
> Register address space 0x0000 – 0x1FFF
> Palette/Tile address space 0x2000 – 0x3FFF
> Gamma_R address space 0x4000 – 0x43FF
> Gamma_G address space 0x4400 – 0x47FF
> Gamma_B address space 0x4800 – 0x4BFF
> Cursor address space 0x4C00 – 0x4FFF
Thanks!^_^
>
> Can I have an Ack from the device tree maintainers here?
>
> <snip>
>
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> > @@ -22,6 +22,22 @@
> > #include "fsl_dcu_drm_drv.h"
> > #include "fsl_dcu_drm_plane.h"
> >
> > +static void fsl_crtc_gamma_set(struct drm_crtc *crtc, struct
> > drm_color_lut *lut,
> > + uint32_t size)
> > +{
> > + struct fsl_dcu_drm_device *fsl_dev = crtc->dev->dev_private;
> > + unsigned int i;
> > +
> > + for (i = 0; i < size; i++) {
> > + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_R + 4 *
> i,
> > + lut[i].red);
> > + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_G + 4
> * i,
> > + lut[i].green);
> > + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_B + 4 *
> i,
> > + lut[i].blue);
> > + }
> > +}
> > +
> > static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> > struct drm_crtc_state *old_crtc_state)
> {
>
> <snip>
>
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> > @@ -48,6 +48,15 @@ static const struct regmap_config
> fsl_dcu_regmap_config = {
> > .volatile_reg = fsl_dcu_drm_is_volatile_reg, };
> >
> > +static const struct regmap_config fsl_dcu_regmap_gamma_config = {
> > + .reg_bits = 32,
> > + .reg_stride = 4,
> > + .val_bits = 32,
> > + .val_format_endian = REGMAP_ENDIAN_LITTLE,
>
> I would like to have a comment here why we force little endian.
>
Oh, I was going to do that, but forgotten anyway.
> > +
> > + .volatile_reg = fsl_dcu_drm_is_volatile_reg, };
> > +
>
> <snip>
>
> > @@ -365,6 +374,25 @@ static int fsl_dcu_drm_probe(struct platform_device
> *pdev)
> > return PTR_ERR(fsl_dev->regmap);
> > }
> >
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "gamma");
> > + if (!res) {
> > + dev_err(dev, "could not get gamma memory resource\n");
> > + return -ENODEV;
> > + }
> > +
> > + base_gamma = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(base_gamma)) {
> > + ret = PTR_ERR(base_gamma);
> > + return ret;
> > + }
> > +
> > + fsl_dev->regmap_gamma = devm_regmap_init_mmio(dev,
> base_gamma,
> > + &fsl_dcu_regmap_config);
> > + if (IS_ERR(fsl_dev->regmap_gamma)) {
> > + dev_err(dev, "regmap_gamma init failed\n");
> > + return PTR_ERR(fsl_dev->regmap_gamma);
> > + }
> > +
>
> Mark, what do you think, is this a reasonable approach to work around this
> errata?
I was thinking is it possible to define a different endianness for each register map.
Best Regards,
Meng
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-07 9:22 [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties Meng Yi
2016-09-07 17:05 ` Stefan Agner
@ 2016-09-13 7:02 ` Stefan Agner
2016-09-13 8:49 ` Meng Yi
1 sibling, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-09-13 7:02 UTC (permalink / raw)
To: Meng Yi; +Cc: jianwei.wang.chn, dri-devel
Hi Meng,
One more thing which I have my concern:
On 2016-09-07 02:22, Meng Yi wrote:
> Gamma correction is optional and can be used to adjust the color
> output values to match the gamut of a particular TFT LCD panel
> Errata:
> Gamma_R, Gamma_G and Gamma_B registers are little-endian registers
> while the rest of the address-space in 2D-ACE is big-endian.
> Workaround:
> Split the DCU regs into "regs", "palette", "gamma" and "cursor".
> Create a second regmap for gamma memory space using little endian.
>
> Suggested-by: Stefan Agner <stefan@agner.ch>
> Signed-off-by: Meng Yi <meng.yi@nxp.com>
> ---
> Changes in V3:
> -created a second regmap for gamma
> -updated the DCU DT binding
> ---
> .../devicetree/bindings/display/fsl,dcu.txt | 6 +++-
> drivers/gpu/drm/fsl-dcu/Kconfig | 6 ++++
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 38 ++++++++++++++++++++++
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 30 ++++++++++++++++-
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 8 +++++
> 5 files changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> index 63ec2a6..1b1321a 100644
> --- a/Documentation/devicetree/bindings/display/fsl,dcu.txt
> +++ b/Documentation/devicetree/bindings/display/fsl,dcu.txt
> @@ -20,7 +20,11 @@ Optional properties:
> Examples:
> dcu: dcu@2ce0000 {
> compatible = "fsl,ls1021a-dcu";
> - reg = <0x0 0x2ce0000 0x0 0x10000>;
> + reg = <0x0 0x2ce0000 0x0 0x2000>,
> + <0x0 0x2ce2000 0x0 0x2000>,
> + <0x0 0x2ce4000 0x0 0xc00>,
> + <0x0 0x2ce4c00 0x0 0x400>;
> + reg-names = "regs", "palette", "gamma", "cursor";
> clocks = <&platform_clk 0>, <&platform_clk 0>;
> clock-names = "dcu", "pix";
> big-endian;
> diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig
> index 14a72c4..f9c76b1 100644
> --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> @@ -11,3 +11,9 @@ config DRM_FSL_DCU
> help
> Choose this option if you have an Freescale DCU chipset.
> If M is selected the module will be called fsl-dcu-drm.
> +
> +config DRM_FSL_DCU_GAMMA
> + bool "Gamma Correction Support for NXP/Freescale DCU"
> + depends on DRM_FSL_DCU
> + help
> + Enable support for gamma correction.
What is the reason for making this a configuration option? Are there
implementation without support for the Gamma tables?
--
Stefan
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> index 3371635..25ab0ff 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
> @@ -22,6 +22,22 @@
> #include "fsl_dcu_drm_drv.h"
> #include "fsl_dcu_drm_plane.h"
>
> +static void fsl_crtc_gamma_set(struct drm_crtc *crtc, struct
> drm_color_lut *lut,
> + uint32_t size)
> +{
> + struct fsl_dcu_drm_device *fsl_dev = crtc->dev->dev_private;
> + unsigned int i;
> +
> + for (i = 0; i < size; i++) {
> + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_R + 4 * i,
> + lut[i].red);
> + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_G + 4 * i,
> + lut[i].green);
> + regmap_write(fsl_dev->regmap_gamma, FSL_GAMMA_B + 4 * i,
> + lut[i].blue);
> + }
> +}
> +
> static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
> @@ -37,6 +53,11 @@ static void fsl_dcu_drm_crtc_atomic_flush(struct
> drm_crtc *crtc,
> drm_crtc_send_vblank_event(crtc, event);
> spin_unlock_irq(&crtc->dev->event_lock);
> }
> +
> + if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
> + fsl_crtc_gamma_set(crtc, (struct drm_color_lut *)
> + crtc->state->gamma_lut->data,
> + 256);
> }
>
> static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
> @@ -46,6 +67,10 @@ static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
>
> drm_crtc_vblank_off(crtc);
>
> + if (fsl_dev->enable_color_mgmt)
> + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> + DCU_MODE_EN_GAMMA_MASK, 0);
> +
> regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> DCU_MODE_DCU_MODE_MASK,
> DCU_MODE_DCU_MODE(DCU_MODE_OFF));
> @@ -58,6 +83,11 @@ static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc)
> struct drm_device *dev = crtc->dev;
> struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
>
> + if (fsl_dev->enable_color_mgmt)
> + regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> + DCU_MODE_EN_GAMMA_MASK,
> + DCU_MODE_GAMMA_ENABLE);
> +
> regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> DCU_MODE_DCU_MODE_MASK,
> DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
> @@ -135,6 +165,7 @@ static const struct drm_crtc_funcs
> fsl_dcu_drm_crtc_funcs = {
> .page_flip = drm_atomic_helper_page_flip,
> .reset = drm_atomic_helper_crtc_reset,
> .set_config = drm_atomic_helper_set_config,
> + .gamma_set = drm_atomic_helper_legacy_gamma_set,
> };
>
> int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
> @@ -158,5 +189,12 @@ int fsl_dcu_drm_crtc_create(struct
> fsl_dcu_drm_device *fsl_dev)
>
> drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs);
>
> +#ifdef CONFIG_DRM_FSL_DCU_GAMMA
> + fsl_dev->enable_color_mgmt = true;
> +
> + drm_crtc_enable_color_mgmt(crtc, 0, false, 256);
> + drm_mode_crtc_set_gamma_size(crtc, 256);
> +#endif
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index 7882387..f70ea68 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -48,6 +48,15 @@ static const struct regmap_config fsl_dcu_regmap_config = {
> .volatile_reg = fsl_dcu_drm_is_volatile_reg,
> };
>
> +static const struct regmap_config fsl_dcu_regmap_gamma_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .val_format_endian = REGMAP_ENDIAN_LITTLE,
> +
> + .volatile_reg = fsl_dcu_drm_is_volatile_reg,
> +};
> +
> static int fsl_dcu_drm_irq_init(struct drm_device *dev)
> {
> struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> @@ -323,7 +332,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
> struct drm_device *drm;
> struct device *dev = &pdev->dev;
> struct resource *res;
> - void __iomem *base;
> + void __iomem *base, *base_gamma;
> struct drm_driver *driver = &fsl_dcu_drm_driver;
> struct clk *pix_clk_in;
> char pix_clk_name[32];
> @@ -365,6 +374,25 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
> return PTR_ERR(fsl_dev->regmap);
> }
>
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma");
> + if (!res) {
> + dev_err(dev, "could not get gamma memory resource\n");
> + return -ENODEV;
> + }
> +
> + base_gamma = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base_gamma)) {
> + ret = PTR_ERR(base_gamma);
> + return ret;
> + }
> +
> + fsl_dev->regmap_gamma = devm_regmap_init_mmio(dev, base_gamma,
> + &fsl_dcu_regmap_config);
> + if (IS_ERR(fsl_dev->regmap_gamma)) {
> + dev_err(dev, "regmap_gamma init failed\n");
> + return PTR_ERR(fsl_dev->regmap_gamma);
> + }
> +
> fsl_dev->clk = devm_clk_get(dev, "dcu");
> if (IS_ERR(fsl_dev->clk)) {
> dev_err(dev, "failed to get dcu clock\n");
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> index 3b371fe7..95427f6 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -25,6 +25,8 @@
> #define DCU_MODE_NORMAL 1
> #define DCU_MODE_TEST 2
> #define DCU_MODE_COLORBAR 3
> +#define DCU_MODE_EN_GAMMA_MASK 0x04
> +#define DCU_MODE_GAMMA_ENABLE BIT(2)
>
> #define DCU_BGND 0x0014
> #define DCU_BGND_R(x) ((x) << 16)
> @@ -165,6 +167,10 @@
> #define VF610_LAYER_REG_NUM 9
> #define LS1021A_LAYER_REG_NUM 10
>
> +#define FSL_GAMMA_R 0x000
> +#define FSL_GAMMA_G 0x400
> +#define FSL_GAMMA_B 0x800
> +
> struct clk;
> struct device;
> struct drm_device;
> @@ -182,6 +188,7 @@ struct fsl_dcu_drm_device {
> struct device *dev;
> struct device_node *np;
> struct regmap *regmap;
> + struct regmap *regmap_gamma;
> int irq;
> struct clk *clk;
> struct clk *pix_clk;
> @@ -195,6 +202,7 @@ struct fsl_dcu_drm_device {
> struct fsl_dcu_drm_connector connector;
> const struct fsl_dcu_soc_data *soc;
> struct drm_atomic_state *state;
> + bool enable_color_mgmt;
> };
>
> void fsl_dcu_fbdev_init(struct drm_device *dev);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-13 7:02 ` Stefan Agner
@ 2016-09-13 8:49 ` Meng Yi
2016-09-21 18:10 ` Stefan Agner
0 siblings, 1 reply; 11+ messages in thread
From: Meng Yi @ 2016-09-13 8:49 UTC (permalink / raw)
To: Stefan Agner; +Cc: jianwei.wang.chn, dri-devel
> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
> > help
> > Choose this option if you have an Freescale DCU chipset.
> > If M is selected the module will be called fsl-dcu-drm.
> > +
> > +config DRM_FSL_DCU_GAMMA
> > + bool "Gamma Correction Support for NXP/Freescale DCU"
> > + depends on DRM_FSL_DCU
> > + help
> > + Enable support for gamma correction.
>
> What is the reason for making this a configuration option? Are there
> implementation without support for the Gamma tables?
>
When gamma correction is enabled, the color won't display normally since
The gamma tables are not filled with correct data. So I give a choice to not using
The gamma correction when you don't want to use it.
Should I remove this configuration?
Best Regards,
Meng
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-13 8:49 ` Meng Yi
@ 2016-09-21 18:10 ` Stefan Agner
2016-09-22 6:29 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Agner @ 2016-09-21 18:10 UTC (permalink / raw)
To: Meng Yi; +Cc: jianwei.wang.chn, dri-devel
On 2016-09-13 01:49, Meng Yi wrote:
>> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
>> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
>> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
>> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
>> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
>> > help
>> > Choose this option if you have an Freescale DCU chipset.
>> > If M is selected the module will be called fsl-dcu-drm.
>> > +
>> > +config DRM_FSL_DCU_GAMMA
>> > + bool "Gamma Correction Support for NXP/Freescale DCU"
>> > + depends on DRM_FSL_DCU
>> > + help
>> > + Enable support for gamma correction.
>>
>> What is the reason for making this a configuration option? Are there
>> implementation without support for the Gamma tables?
>>
> When gamma correction is enabled, the color won't display normally since
> The gamma tables are not filled with correct data. So I give a choice
> to not using
> The gamma correction when you don't want to use it.
>
> Should I remove this configuration?
Yeah making this a compile time configuration seems wrong to me.
I guess we should fill the table with a reasonable default then. The
omapdrm driver seems to do something similar.
Best regards,
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-21 18:10 ` Stefan Agner
@ 2016-09-22 6:29 ` Daniel Vetter
2016-09-26 6:04 ` Meng Yi
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2016-09-22 6:29 UTC (permalink / raw)
To: Stefan Agner; +Cc: jianwei.wang.chn, Meng Yi, dri-devel
On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote:
> On 2016-09-13 01:49, Meng Yi wrote:
> >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
> >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
> >> > help
> >> > Choose this option if you have an Freescale DCU chipset.
> >> > If M is selected the module will be called fsl-dcu-drm.
> >> > +
> >> > +config DRM_FSL_DCU_GAMMA
> >> > + bool "Gamma Correction Support for NXP/Freescale DCU"
> >> > + depends on DRM_FSL_DCU
> >> > + help
> >> > + Enable support for gamma correction.
> >>
> >> What is the reason for making this a configuration option? Are there
> >> implementation without support for the Gamma tables?
> >>
> > When gamma correction is enabled, the color won't display normally since
> > The gamma tables are not filled with correct data. So I give a choice
> > to not using
> > The gamma correction when you don't want to use it.
> >
> > Should I remove this configuration?
>
> Yeah making this a compile time configuration seems wrong to me.
>
> I guess we should fill the table with a reasonable default then. The
> omapdrm driver seems to do something similar.
Yup Kconfig is definitely not the correct way to fix a setup problem in
your driver. Note that even when you enable gamma lut userspace is allowed
to set a NULL table, and in that case you need to either disable the gamma
lut, or fill it with a 1:1 mapping (if you hw doesn't have a disable
flag). At boot-up the table will also be NULL (except when you have
special setup code), and if that results in a black screen your driver is
buggy. Papering over a driver bug with Kconfig is definitely not ok.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-22 6:29 ` Daniel Vetter
@ 2016-09-26 6:04 ` Meng Yi
2016-09-26 7:59 ` Daniel Vetter
2016-09-26 19:33 ` Stefan Agner
0 siblings, 2 replies; 11+ messages in thread
From: Meng Yi @ 2016-09-26 6:04 UTC (permalink / raw)
To: Daniel Vetter, Stefan Agner; +Cc: jianwei.wang.chn, dri-devel
> On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote:
> > On 2016-09-13 01:49, Meng Yi wrote:
> > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
> > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
> > >> > help
> > >> > Choose this option if you have an Freescale DCU chipset.
> > >> > If M is selected the module will be called fsl-dcu-drm.
> > >> > +
> > >> > +config DRM_FSL_DCU_GAMMA
> > >> > + bool "Gamma Correction Support for NXP/Freescale DCU"
> > >> > + depends on DRM_FSL_DCU
> > >> > + help
> > >> > + Enable support for gamma correction.
> > >>
> > >> What is the reason for making this a configuration option? Are
> > >> there implementation without support for the Gamma tables?
> > >>
> > > When gamma correction is enabled, the color won't display normally
> > > since The gamma tables are not filled with correct data. So I give a
> > > choice to not using The gamma correction when you don't want to use
> > > it.
> > >
> > > Should I remove this configuration?
> >
> > Yeah making this a compile time configuration seems wrong to me.
> >
> > I guess we should fill the table with a reasonable default then. The
> > omapdrm driver seems to do something similar.
>
> Yup Kconfig is definitely not the correct way to fix a setup problem in your
> driver. Note that even when you enable gamma lut userspace is allowed to set
> a NULL table, and in that case you need to either disable the gamma lut, or fill it
> with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table
> will also be NULL (except when you have special setup code), and if that results
> in a black screen your driver is buggy. Papering over a driver bug with Kconfig is
> definitely not ok.
How about initialize the tables with static array(such as gmma = 2.2, etc)
/*gamma = 2.2*/
u32 gamma_default_lut[] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02,
0x02, 0x02, 0x02, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x04, 0x04, 0x04, 0x05, 0x05, 0x05,
0x06, 0x06, 0x06, 0x07, 0x07, 0x07, 0x08, 0x08, 0x08, 0x09, 0x09, 0x09, 0x0A, 0x0A, 0x0B, 0x0B,
0x0B, 0x0C, 0x0C, 0x0D, 0x0D, 0x0E, 0x0E, 0x0E, 0x0F, 0x0F, 0x10, 0x10, 0x11, 0x11, 0x12, 0x13,
0x13, 0x14, 0x14, 0x15, 0x15, 0x16, 0x17, 0x17, 0x18, 0x18, 0x19, 0x1A, 0x1A, 0x1B, 0x1C, 0x1C,
0x1D, 0x1E, 0x1E, 0x1F, 0x20, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25, 0x26, 0x27, 0x27, 0x28,
0x29, 0x2A, 0x2B, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32, 0x32, 0x33, 0x34, 0x35, 0x36,
0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F, 0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47,
0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4F, 0x50, 0x51, 0x52, 0x53, 0x54, 0x56, 0x57, 0x58, 0x59,
0x5B, 0x5C, 0x5D, 0x5E, 0x60, 0x61, 0x62, 0x64, 0x65, 0x66, 0x68, 0x69, 0x6A, 0x6C, 0x6D, 0x6F,
0x70, 0x71, 0x73, 0x74, 0x76, 0x77, 0x79, 0x7A, 0x7C, 0x7D, 0x7F, 0x80, 0x82, 0x83, 0x85, 0x86,
0x88, 0x89, 0x8B, 0x8C, 0x8E, 0x90, 0x91, 0x93, 0x95, 0x96, 0x98, 0x9A, 0x9B, 0x9D, 0x9F, 0xA0,
0xA2, 0xA4, 0xA5, 0xA7, 0xA9, 0xAB, 0xAC, 0xAE, 0xB0, 0xB2, 0xB4, 0xB6, 0xB7, 0xB9, 0xBB, 0xBD,
0xBF, 0xC1, 0xC3, 0xC4, 0xC6, 0xC8, 0xCA, 0xCC, 0xCE, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC,
0xDE, 0xE0, 0xE2, 0xE4, 0xE6, 0xE8, 0xEB, 0xED, 0xEF, 0xF1, 0xF3, 0xF5, 0xF7, 0xFA, 0xFC, 0xFE,
};
static void fsl_dcu_drm_init_gamma_tables(struct drm_crtc *crtc, uint32_t size)
{
unsigned int i;
struct drm_color_lut lut[size];
for (i = 0; i < size; i++) {
lut[i].red = gamma_default_lut[i];
lut[i].green = gamma_default_lut[i];
lut[i].blue = gamma_default_lut[i];
}
fsl_crtc_gamma_set(crtc, lut, size);
}
And call "fsl_dcu_drm_init_gamma_tables" within "fsl_dcu_drm_crtc_create".
But it seems not graceful putting a static array within the source code. Or generate that array within "fsl_dcu_drm_init_gamma_tables"?
Best Regards,
Meng
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-26 6:04 ` Meng Yi
@ 2016-09-26 7:59 ` Daniel Vetter
2016-09-26 8:17 ` Ville Syrjälä
2016-09-26 19:33 ` Stefan Agner
1 sibling, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2016-09-26 7:59 UTC (permalink / raw)
To: Meng Yi; +Cc: jianwei.wang.chn, dri-devel
On Mon, Sep 26, 2016 at 06:04:09AM +0000, Meng Yi wrote:
>
>
> > On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote:
> > > On 2016-09-13 01:49, Meng Yi wrote:
> > > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> > > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
> > > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> > > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> > > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
> > > >> > help
> > > >> > Choose this option if you have an Freescale DCU chipset.
> > > >> > If M is selected the module will be called fsl-dcu-drm.
> > > >> > +
> > > >> > +config DRM_FSL_DCU_GAMMA
> > > >> > + bool "Gamma Correction Support for NXP/Freescale DCU"
> > > >> > + depends on DRM_FSL_DCU
> > > >> > + help
> > > >> > + Enable support for gamma correction.
> > > >>
> > > >> What is the reason for making this a configuration option? Are
> > > >> there implementation without support for the Gamma tables?
> > > >>
> > > > When gamma correction is enabled, the color won't display normally
> > > > since The gamma tables are not filled with correct data. So I give a
> > > > choice to not using The gamma correction when you don't want to use
> > > > it.
> > > >
> > > > Should I remove this configuration?
> > >
> > > Yeah making this a compile time configuration seems wrong to me.
> > >
> > > I guess we should fill the table with a reasonable default then. The
> > > omapdrm driver seems to do something similar.
> >
> > Yup Kconfig is definitely not the correct way to fix a setup problem in your
> > driver. Note that even when you enable gamma lut userspace is allowed to set
> > a NULL table, and in that case you need to either disable the gamma lut, or fill it
> > with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table
> > will also be NULL (except when you have special setup code), and if that results
> > in a black screen your driver is buggy. Papering over a driver bug with Kconfig is
> > definitely not ok.
>
> How about initialize the tables with static array(such as gmma = 2.2, etc)
Linear ramp as the default (which is dead easy to generate) should be good
enough imo, but I guess you can go with something else too.
-Daniel
>
> /*gamma = 2.2*/
> u32 gamma_default_lut[] = {
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02,
> 0x02, 0x02, 0x02, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x04, 0x04, 0x04, 0x05, 0x05, 0x05,
> 0x06, 0x06, 0x06, 0x07, 0x07, 0x07, 0x08, 0x08, 0x08, 0x09, 0x09, 0x09, 0x0A, 0x0A, 0x0B, 0x0B,
> 0x0B, 0x0C, 0x0C, 0x0D, 0x0D, 0x0E, 0x0E, 0x0E, 0x0F, 0x0F, 0x10, 0x10, 0x11, 0x11, 0x12, 0x13,
> 0x13, 0x14, 0x14, 0x15, 0x15, 0x16, 0x17, 0x17, 0x18, 0x18, 0x19, 0x1A, 0x1A, 0x1B, 0x1C, 0x1C,
> 0x1D, 0x1E, 0x1E, 0x1F, 0x20, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25, 0x26, 0x27, 0x27, 0x28,
> 0x29, 0x2A, 0x2B, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32, 0x32, 0x33, 0x34, 0x35, 0x36,
> 0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F, 0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47,
> 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4F, 0x50, 0x51, 0x52, 0x53, 0x54, 0x56, 0x57, 0x58, 0x59,
> 0x5B, 0x5C, 0x5D, 0x5E, 0x60, 0x61, 0x62, 0x64, 0x65, 0x66, 0x68, 0x69, 0x6A, 0x6C, 0x6D, 0x6F,
> 0x70, 0x71, 0x73, 0x74, 0x76, 0x77, 0x79, 0x7A, 0x7C, 0x7D, 0x7F, 0x80, 0x82, 0x83, 0x85, 0x86,
> 0x88, 0x89, 0x8B, 0x8C, 0x8E, 0x90, 0x91, 0x93, 0x95, 0x96, 0x98, 0x9A, 0x9B, 0x9D, 0x9F, 0xA0,
> 0xA2, 0xA4, 0xA5, 0xA7, 0xA9, 0xAB, 0xAC, 0xAE, 0xB0, 0xB2, 0xB4, 0xB6, 0xB7, 0xB9, 0xBB, 0xBD,
> 0xBF, 0xC1, 0xC3, 0xC4, 0xC6, 0xC8, 0xCA, 0xCC, 0xCE, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC,
> 0xDE, 0xE0, 0xE2, 0xE4, 0xE6, 0xE8, 0xEB, 0xED, 0xEF, 0xF1, 0xF3, 0xF5, 0xF7, 0xFA, 0xFC, 0xFE,
> };
>
> static void fsl_dcu_drm_init_gamma_tables(struct drm_crtc *crtc, uint32_t size)
> {
> unsigned int i;
> struct drm_color_lut lut[size];
>
> for (i = 0; i < size; i++) {
> lut[i].red = gamma_default_lut[i];
> lut[i].green = gamma_default_lut[i];
> lut[i].blue = gamma_default_lut[i];
> }
>
> fsl_crtc_gamma_set(crtc, lut, size);
> }
>
> And call "fsl_dcu_drm_init_gamma_tables" within "fsl_dcu_drm_crtc_create".
>
> But it seems not graceful putting a static array within the source code. Or generate that array within "fsl_dcu_drm_init_gamma_tables"?
>
> Best Regards,
> Meng
>
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-26 7:59 ` Daniel Vetter
@ 2016-09-26 8:17 ` Ville Syrjälä
0 siblings, 0 replies; 11+ messages in thread
From: Ville Syrjälä @ 2016-09-26 8:17 UTC (permalink / raw)
To: Daniel Vetter; +Cc: jianwei.wang.chn, Meng Yi, dri-devel
On Mon, Sep 26, 2016 at 09:59:25AM +0200, Daniel Vetter wrote:
> On Mon, Sep 26, 2016 at 06:04:09AM +0000, Meng Yi wrote:
> >
> >
> > > On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote:
> > > > On 2016-09-13 01:49, Meng Yi wrote:
> > > > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> > > > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
> > > > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> > > > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> > > > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
> > > > >> > help
> > > > >> > Choose this option if you have an Freescale DCU chipset.
> > > > >> > If M is selected the module will be called fsl-dcu-drm.
> > > > >> > +
> > > > >> > +config DRM_FSL_DCU_GAMMA
> > > > >> > + bool "Gamma Correction Support for NXP/Freescale DCU"
> > > > >> > + depends on DRM_FSL_DCU
> > > > >> > + help
> > > > >> > + Enable support for gamma correction.
> > > > >>
> > > > >> What is the reason for making this a configuration option? Are
> > > > >> there implementation without support for the Gamma tables?
> > > > >>
> > > > > When gamma correction is enabled, the color won't display normally
> > > > > since The gamma tables are not filled with correct data. So I give a
> > > > > choice to not using The gamma correction when you don't want to use
> > > > > it.
> > > > >
> > > > > Should I remove this configuration?
> > > >
> > > > Yeah making this a compile time configuration seems wrong to me.
> > > >
> > > > I guess we should fill the table with a reasonable default then. The
> > > > omapdrm driver seems to do something similar.
> > >
> > > Yup Kconfig is definitely not the correct way to fix a setup problem in your
> > > driver. Note that even when you enable gamma lut userspace is allowed to set
> > > a NULL table, and in that case you need to either disable the gamma lut, or fill it
> > > with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table
> > > will also be NULL (except when you have special setup code), and if that results
> > > in a black screen your driver is buggy. Papering over a driver bug with Kconfig is
> > > definitely not ok.
> >
> > How about initialize the tables with static array(such as gmma = 2.2, etc)
>
> Linear ramp as the default (which is dead easy to generate) should be good
> enough imo, but I guess you can go with something else too.
Linear would seem like a sane default choice to me as photos/videos etc. are
typically stored with gamma included. And when the compositor/etc. is not gamma
aware you might not want to apply the gamma twice for such things.
> -Daniel
>
> >
> > /*gamma = 2.2*/
> > u32 gamma_default_lut[] = {
> > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02,
> > 0x02, 0x02, 0x02, 0x02, 0x03, 0x03, 0x03, 0x03, 0x03, 0x04, 0x04, 0x04, 0x04, 0x05, 0x05, 0x05,
> > 0x06, 0x06, 0x06, 0x07, 0x07, 0x07, 0x08, 0x08, 0x08, 0x09, 0x09, 0x09, 0x0A, 0x0A, 0x0B, 0x0B,
> > 0x0B, 0x0C, 0x0C, 0x0D, 0x0D, 0x0E, 0x0E, 0x0E, 0x0F, 0x0F, 0x10, 0x10, 0x11, 0x11, 0x12, 0x13,
> > 0x13, 0x14, 0x14, 0x15, 0x15, 0x16, 0x17, 0x17, 0x18, 0x18, 0x19, 0x1A, 0x1A, 0x1B, 0x1C, 0x1C,
> > 0x1D, 0x1E, 0x1E, 0x1F, 0x20, 0x20, 0x21, 0x22, 0x23, 0x23, 0x24, 0x25, 0x26, 0x27, 0x27, 0x28,
> > 0x29, 0x2A, 0x2B, 0x2B, 0x2C, 0x2D, 0x2E, 0x2F, 0x30, 0x31, 0x32, 0x32, 0x33, 0x34, 0x35, 0x36,
> > 0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F, 0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47,
> > 0x48, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x4F, 0x50, 0x51, 0x52, 0x53, 0x54, 0x56, 0x57, 0x58, 0x59,
> > 0x5B, 0x5C, 0x5D, 0x5E, 0x60, 0x61, 0x62, 0x64, 0x65, 0x66, 0x68, 0x69, 0x6A, 0x6C, 0x6D, 0x6F,
> > 0x70, 0x71, 0x73, 0x74, 0x76, 0x77, 0x79, 0x7A, 0x7C, 0x7D, 0x7F, 0x80, 0x82, 0x83, 0x85, 0x86,
> > 0x88, 0x89, 0x8B, 0x8C, 0x8E, 0x90, 0x91, 0x93, 0x95, 0x96, 0x98, 0x9A, 0x9B, 0x9D, 0x9F, 0xA0,
> > 0xA2, 0xA4, 0xA5, 0xA7, 0xA9, 0xAB, 0xAC, 0xAE, 0xB0, 0xB2, 0xB4, 0xB6, 0xB7, 0xB9, 0xBB, 0xBD,
> > 0xBF, 0xC1, 0xC3, 0xC4, 0xC6, 0xC8, 0xCA, 0xCC, 0xCE, 0xD0, 0xD2, 0xD4, 0xD6, 0xD8, 0xDA, 0xDC,
> > 0xDE, 0xE0, 0xE2, 0xE4, 0xE6, 0xE8, 0xEB, 0xED, 0xEF, 0xF1, 0xF3, 0xF5, 0xF7, 0xFA, 0xFC, 0xFE,
> > };
> >
> > static void fsl_dcu_drm_init_gamma_tables(struct drm_crtc *crtc, uint32_t size)
> > {
> > unsigned int i;
> > struct drm_color_lut lut[size];
> >
> > for (i = 0; i < size; i++) {
> > lut[i].red = gamma_default_lut[i];
> > lut[i].green = gamma_default_lut[i];
> > lut[i].blue = gamma_default_lut[i];
> > }
> >
> > fsl_crtc_gamma_set(crtc, lut, size);
> > }
> >
> > And call "fsl_dcu_drm_init_gamma_tables" within "fsl_dcu_drm_crtc_create".
> >
> > But it seems not graceful putting a static array within the source code. Or generate that array within "fsl_dcu_drm_init_gamma_tables"?
> >
> > Best Regards,
> > Meng
> >
> > > -Daniel
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties
2016-09-26 6:04 ` Meng Yi
2016-09-26 7:59 ` Daniel Vetter
@ 2016-09-26 19:33 ` Stefan Agner
1 sibling, 0 replies; 11+ messages in thread
From: Stefan Agner @ 2016-09-26 19:33 UTC (permalink / raw)
To: Meng Yi; +Cc: jianwei.wang.chn, dri-devel
On 2016-09-25 23:04, Meng Yi wrote:
>> On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote:
>> > On 2016-09-13 01:49, Meng Yi wrote:
>> > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
>> > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
>> > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
>> > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
>> > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
>> > >> > help
>> > >> > Choose this option if you have an Freescale DCU chipset.
>> > >> > If M is selected the module will be called fsl-dcu-drm.
>> > >> > +
>> > >> > +config DRM_FSL_DCU_GAMMA
>> > >> > + bool "Gamma Correction Support for NXP/Freescale DCU"
>> > >> > + depends on DRM_FSL_DCU
>> > >> > + help
>> > >> > + Enable support for gamma correction.
>> > >>
>> > >> What is the reason for making this a configuration option? Are
>> > >> there implementation without support for the Gamma tables?
>> > >>
>> > > When gamma correction is enabled, the color won't display normally
>> > > since The gamma tables are not filled with correct data. So I give a
>> > > choice to not using The gamma correction when you don't want to use
>> > > it.
>> > >
>> > > Should I remove this configuration?
>> >
>> > Yeah making this a compile time configuration seems wrong to me.
>> >
>> > I guess we should fill the table with a reasonable default then. The
>> > omapdrm driver seems to do something similar.
>>
>> Yup Kconfig is definitely not the correct way to fix a setup problem in your
>> driver. Note that even when you enable gamma lut userspace is allowed to set
>> a NULL table, and in that case you need to either disable the gamma lut, or fill it
>> with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table
>> will also be NULL (except when you have special setup code), and if that results
>> in a black screen your driver is buggy. Papering over a driver bug with Kconfig is
>> definitely not ok.
>
> How about initialize the tables with static array(such as gmma = 2.2, etc)
The hardware allows to disable gamma, so I would rather prefer to only
enable it when a valid table is set, and disable gamma at startup and
when a NULL table is set. This also makes sure what we get the least
interference with what we are doing now.
--
Stefan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-09-26 19:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 9:22 [PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties Meng Yi
2016-09-07 17:05 ` Stefan Agner
2016-09-08 2:45 ` Meng Yi
2016-09-13 7:02 ` Stefan Agner
2016-09-13 8:49 ` Meng Yi
2016-09-21 18:10 ` Stefan Agner
2016-09-22 6:29 ` Daniel Vetter
2016-09-26 6:04 ` Meng Yi
2016-09-26 7:59 ` Daniel Vetter
2016-09-26 8:17 ` Ville Syrjälä
2016-09-26 19:33 ` Stefan Agner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.