RE: [PATCH] drm/fsl-dcu: Add gamma set for crtc

[Meng Yi <meng.yi@nxp.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel