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

[Stefan Agner <stefan@agner.ch>]

On 2016-09-04 19:28, Meng Yi wrote:
> 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/
> 

Oh sorry, missed that patch. However, the discussion below is still
valid:

> ...
> 
>> > +	};
>> > +
>> > +	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?

Do you have the exact wording of the errata?

There are two problems to the current approach: First, the two byte
swaps (yours and then the byte swap due to the big-endian configuration
of regmap) seems ugly to me. Second, the gamma value is the lowest byte
in Vybrid, and on Vybrid the regmap is configured little-endian. Hence
your code won't work on Vybrid... Therefor I still think that using a
second regmap would be nicer.

--
Stefan




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel