[PATCH] drm/fsl-dcu: Add gamma set for crtc
Meng Yi
meng.yi at nxp.com
Mon Sep 5 02:28:27 UTC 2016
Hi Stefan,
> > + */
> > +static u32 swap_bytes(u16 var)
>
> We certainly don't want a byte swapping function in the driver. I am sure Linux
> has appropriate functions for that already, however, I am not convinced that
> we need that at all.
>
Yeah, sure. Actually I had sent the V2 for this feature, which just using "<<24" to do this
https://patchwork.kernel.org/patch/9252389/
...
> > + };
> > +
> > + struct drm_device *dev = crtc->dev;
> > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> > + unsigned int i;
> > + struct rgb glut;
> > +
> > + for (i = 0; i < size; i++) {
> > + glut.r[i] = swap_bytes(r[i]);
> > + glut.g[i] = swap_bytes(g[i]);
> > + glut.b[i] = swap_bytes(b[i]);
> > + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i,
> glut.r[i]);
> > + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i,
> glut.g[i]);
> > + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i,
> glut.b[i]);
>
> I guess the problem is that regmap_write does byte swapping because
> ls1021a.dtsi defines the whole DCU register space to be big-endian. So you end
> up doing byte swapping twice.
>
> If the gamma area is really little-endian, then DCU on LS1021a seems to be
> quite a mess... :-(
>
> In this case, we probably should create a second regmap for the different areas
> specifically, e.g. change the device tree:
>
> reg = <0x0 0x2ce0000 0x0 0x2000
> 0x0 0x2ce2000 0x0 0x2000
> 0x0 0x2ce4000 0x0 0xc00
> 0x0 0x2ce4c00 0x0 0x400>;
>
> reg-names = "regs", "palette", "gamma", "cursor";
> i
> /* Use Gamma is always little endian */
> static const struct regmap_config fsl_dcu_regmap_gamma_config = { ...
> .val_format_endian = REGMAP_ENDIAN_LITTLE, ...
> };
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma");
> base_gamma = devm_ioremap_resource(dev, res);
>
> fsl_dev->regmap_gamma = devm_regmap_init_mmio(...)
>
>
> regmap_write(fsl_dev->regmap_gamma, ...)
>
This is a errta for DCU, Gamma registers are supposed to be big endian, but actually it is little endian, do we
Really need to create a second regmap?
>
> @Mark, what do you think? Do we have a (better) solution for such cases?
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = {
> > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> > @@ -135,6 +197,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 = fsl_crtc_gamma_set,
> > };
> >
> > int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) 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..d3bc540 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,9 @@
> > #define DCU_MODE_NORMAL 1
> > #define DCU_MODE_TEST 2
> > #define DCU_MODE_COLORBAR 3
> > +#define DCU_MODE_EN_GAMMA_MASK 0x04
>
> Nit: In cases where MASK is a single bit, you can use BIT(..)...
>
> > +#define DCU_MODE_GAMMA_ENABLE BIT(2)
> > +#define DCU_MODE_GAMMA_DISABLE 0
>
> That sounds like a useless define to me. In the disable case, just use 0 in
> regmap_update_bits. The .._MASK shows which bit you clear.
>
Yeah, sure. Thanks.
Best Regards,
Meng
More information about the dri-devel
mailing list