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@xxxxxxxx>
> Signed-off-by: Meng Yi <meng.yi@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
