mbox series

[v4,0/5] drm: Add driver for PowerPC OF displays

Message ID 20220928105010.18880-1-tzimmermann@suse.de
Headers show
Series drm: Add driver for PowerPC OF displays | expand

Message

Thomas Zimmermann Sept. 28, 2022, 10:50 a.m. UTC 
PowerPC's Open Firmware offers a simple display buffer for graphics
output. Add ofdrm, a DRM driver for the device. As with the existing
simpledrm driver, the graphics hardware is pre-initialized by the
firmware. The driver only provides blitting, no actual DRM modesetting
is possible.

Patch 1 adds ofdrm, which has again been significantly reworked.
The FWFB library has been removed infavor of various functions in
existing DRM helper libraries. Ofdrm now supports damage iterators
and synchronization for imported GEM BOs.

Patches 2 to 4 add support for color management. The code has been
taken from fbdev's offb. I have no hardware available for testing the
functionality. Qemu's stdvga apparently does not support gamma tables
in RGB modes. I verified that the color management code is executed
by running Gnome's night-mode settings, but the display's color tone
does not change.

Patch 5, which is new in version 4 of this patchset, adds support for
big-endian scanout buffers. It works at least with qemu's ppc64
emulation. Fbdev emulation and pixman rendering works. GL rendering
produces incorrect colors.

Tested by running fbdev emulation, Wayland Gnome, and Weston on qemu's
ppc64le and ppc64 emulation. 

Thomas Zimmermann (5):
  drm/ofdrm: Add ofdrm for Open Firmware framebuffers
  drm/ofdrm: Add CRTC state
  drm/ofdrm: Add per-model device function
  drm/ofdrm: Support color management
  drm/ofdrm: Support big-endian scanout buffers

 MAINTAINERS                         |    1 +
 drivers/gpu/drm/drm_format_helper.c |   10 +
 drivers/gpu/drm/tiny/Kconfig        |   13 +
 drivers/gpu/drm/tiny/Makefile       |    1 +
 drivers/gpu/drm/tiny/ofdrm.c        | 1421 +++++++++++++++++++++++++++
 drivers/video/fbdev/Kconfig         |    1 +
 6 files changed, 1447 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/ofdrm.c


base-commit: eee1f4330f388247943e97b93008ef11ababfda0

Comments

Michal Suchanek Sept. 28, 2022, 11:12 a.m. UTC | #1 
Hello,

On Wed, Sep 28, 2022 at 12:50:10PM +0200, Thomas Zimmermann wrote:
> All DRM formats assume little-endian byte order. On big-endian systems,
> it is likely that the scanout buffer is in big endian as well. Update
> the format accordingly and add endianess conversion to the format-helper
> library. Also opt-in to allocated buffers in host format by default.

This sounds backwards to me.

Skimming through the code it sounds like the buffer is in fact in the
same format all the time but when the CPU is switched to BE it sees the
data loaded from it differently.

Or am I missing something?

Thanks

Michal

> 
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 10 ++++++
>  drivers/gpu/drm/tiny/ofdrm.c        | 55 +++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 4afc4ac27342..fca7936db083 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -659,6 +659,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
>  			drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, false);
>  			return 0;
>  		}
> +	} else if (dst_format == (DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN)) {
> +		if (fb_format == DRM_FORMAT_RGB565) {
> +			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
> +			return 0;
> +		}
>  	} else if (dst_format == DRM_FORMAT_RGB888) {
>  		if (fb_format == DRM_FORMAT_XRGB8888) {
>  			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
> @@ -677,6 +682,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
>  			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
>  			return 0;
>  		}
> +	} else if (dst_format == DRM_FORMAT_BGRX8888) {
> +		if (fb_format == DRM_FORMAT_XRGB8888) {
> +			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
> +			return 0;
> +		}
>  	}
>  
>  	drm_warn_once(fb->dev, "No conversion helper from %p4cc to %p4cc found.\n",
> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
> index 0bf5eebf6678..6e100a7f5db7 100644
> --- a/drivers/gpu/drm/tiny/ofdrm.c
> +++ b/drivers/gpu/drm/tiny/ofdrm.c
> @@ -94,7 +94,7 @@ static int display_get_validated_int0(struct drm_device *dev, const char *name,
>  }
>  
>  static const struct drm_format_info *display_get_validated_format(struct drm_device *dev,
> -								  u32 depth)
> +								  u32 depth, bool big_endian)
>  {
>  	const struct drm_format_info *info;
>  	u32 format;
> @@ -115,6 +115,29 @@ static const struct drm_format_info *display_get_validated_format(struct drm_dev
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	/*
> +	 * DRM formats assume little-endian byte order. Update the format
> +	 * if the scanout buffer uses big-endian ordering.
> +	 */
> +	if (big_endian) {
> +		switch (format) {
> +		case DRM_FORMAT_XRGB8888:
> +			format = DRM_FORMAT_BGRX8888;
> +			break;
> +		case DRM_FORMAT_ARGB8888:
> +			format = DRM_FORMAT_BGRA8888;
> +			break;
> +		case DRM_FORMAT_RGB565:
> +			format = DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN;
> +			break;
> +		case DRM_FORMAT_XRGB1555:
> +			format = DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
>  	info = drm_format_info(format);
>  	if (!info) {
>  		drm_err(dev, "cannot find framebuffer format for depth %u\n", depth);
> @@ -134,6 +157,23 @@ static int display_read_u32_of(struct drm_device *dev, struct device_node *of_no
>  	return ret;
>  }
>  
> +static bool display_get_big_endian_of(struct drm_device *dev, struct device_node *of_node)
> +{
> +	bool big_endian;
> +
> +#ifdef __BIG_ENDIAN
> +	big_endian = true;
> +	if (of_get_property(of_node, "little-endian", NULL))
> +		big_endian = false;
> +#else
> +	big_endian = false;
> +	if (of_get_property(of_node, "big-endian", NULL))
> +		big_endian = true;
> +#endif
> +
> +	return big_endian;
> +}
> +
>  static int display_get_width_of(struct drm_device *dev, struct device_node *of_node)
>  {
>  	u32 width;
> @@ -613,6 +653,7 @@ static void ofdrm_device_set_gamma_linear(struct ofdrm_device *odev,
>  
>  	switch (format->format) {
>  	case DRM_FORMAT_RGB565:
> +	case DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN:
>  		/* Use better interpolation, to take 32 values from 0 to 255 */
>  		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE / 8; i++) {
>  			unsigned char r = i * 8 + i / 4;
> @@ -631,6 +672,7 @@ static void ofdrm_device_set_gamma_linear(struct ofdrm_device *odev,
>  		}
>  		break;
>  	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_BGRX8888:
>  		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE; i++)
>  			odev->funcs->cmap_write(odev, i, i, i, i);
>  		break;
> @@ -650,6 +692,7 @@ static void ofdrm_device_set_gamma(struct ofdrm_device *odev,
>  
>  	switch (format->format) {
>  	case DRM_FORMAT_RGB565:
> +	case DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN:
>  		/* Use better interpolation, to take 32 values from lut[0] to lut[255] */
>  		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE / 8; i++) {
>  			unsigned char r = lut[i * 8 + i / 4].red >> 8;
> @@ -668,6 +711,7 @@ static void ofdrm_device_set_gamma(struct ofdrm_device *odev,
>  		}
>  		break;
>  	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_BGRX8888:
>  		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE; i++) {
>  			unsigned char r = lut[i].red >> 8;
>  			unsigned char g = lut[i].green >> 8;
> @@ -718,6 +762,9 @@ static const uint32_t ofdrm_primary_plane_formats[] = {
>  	DRM_FORMAT_RGB565,
>  	//DRM_FORMAT_XRGB1555,
>  	//DRM_FORMAT_C8,
> +	/* Big-endian formats below */
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN,
>  };
>  
>  static const uint64_t ofdrm_primary_plane_format_modifiers[] = {
> @@ -1048,6 +1095,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  	struct ofdrm_device *odev;
>  	struct drm_device *dev;
>  	enum ofdrm_model model;
> +	bool big_endian;
>  	int width, height, depth, linebytes;
>  	const struct drm_format_info *format;
>  	u64 address;
> @@ -1109,6 +1157,8 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  		break;
>  	}
>  
> +	big_endian = display_get_big_endian_of(dev, of_node);
> +
>  	width = display_get_width_of(dev, of_node);
>  	if (width < 0)
>  		return ERR_PTR(width);
> @@ -1122,7 +1172,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  	if (linebytes < 0)
>  		return ERR_PTR(linebytes);
>  
> -	format = display_get_validated_format(dev, depth);
> +	format = display_get_validated_format(dev, depth, big_endian);
>  	if (IS_ERR(format))
>  		return ERR_CAST(format);
>  	if (!linebytes) {
> @@ -1234,6 +1284,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  		dev->mode_config.preferred_depth = depth;
>  		break;
>  	}
> +	dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>  
>  	/* Primary plane */
>  
> -- 
> 2.37.3
>
Thomas Zimmermann Sept. 28, 2022, 11:30 a.m. UTC | #2 
Hi

Am 28.09.22 um 13:12 schrieb Michal Suchánek:
> Hello,
> 
> On Wed, Sep 28, 2022 at 12:50:10PM +0200, Thomas Zimmermann wrote:
>> All DRM formats assume little-endian byte order. On big-endian systems,
>> it is likely that the scanout buffer is in big endian as well. Update
>> the format accordingly and add endianess conversion to the format-helper
>> library. Also opt-in to allocated buffers in host format by default.
> 
> This sounds backwards to me.

In which way?

> 
> Skimming through the code it sounds like the buffer is in fact in the
> same format all the time but when the CPU is switched to BE it sees the
> data loaded from it differently.
> 
> Or am I missing something?

Which buffer do you mean? The scanout buffer coming from the firmware, 
or the GEM BOs that are allocated by renderers?

The scanout buffer is either in BE or LE format. According to the code 
in offb, it's signaled by a node in the device tree. I took that code 
into ofdrm and set the scanout format accordingly.

The GEM BO can be in any format. If necessary, ofdrm converts internally 
while copying it to the scanout buffer. The quirk we opt in, makes DRM 
prefer whatever default byteorder the host prefers (BE or LE). According 
to the docs, it's the right thing to do. But that only affects the GEM 
code, not the scanout buffer.

Best regards
Thomas

> 
> Thanks
> 
> Michal
> 
>>
>> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 10 ++++++
>>   drivers/gpu/drm/tiny/ofdrm.c        | 55 +++++++++++++++++++++++++++--
>>   2 files changed, 63 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 4afc4ac27342..fca7936db083 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -659,6 +659,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
>>   			drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, false);
>>   			return 0;
>>   		}
>> +	} else if (dst_format == (DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN)) {
>> +		if (fb_format == DRM_FORMAT_RGB565) {
>> +			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
>> +			return 0;
>> +		}
>>   	} else if (dst_format == DRM_FORMAT_RGB888) {
>>   		if (fb_format == DRM_FORMAT_XRGB8888) {
>>   			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
>> @@ -677,6 +682,11 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
>>   			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
>>   			return 0;
>>   		}
>> +	} else if (dst_format == DRM_FORMAT_BGRX8888) {
>> +		if (fb_format == DRM_FORMAT_XRGB8888) {
>> +			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
>> +			return 0;
>> +		}
>>   	}
>>   
>>   	drm_warn_once(fb->dev, "No conversion helper from %p4cc to %p4cc found.\n",
>> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
>> index 0bf5eebf6678..6e100a7f5db7 100644
>> --- a/drivers/gpu/drm/tiny/ofdrm.c
>> +++ b/drivers/gpu/drm/tiny/ofdrm.c
>> @@ -94,7 +94,7 @@ static int display_get_validated_int0(struct drm_device *dev, const char *name,
>>   }
>>   
>>   static const struct drm_format_info *display_get_validated_format(struct drm_device *dev,
>> -								  u32 depth)
>> +								  u32 depth, bool big_endian)
>>   {
>>   	const struct drm_format_info *info;
>>   	u32 format;
>> @@ -115,6 +115,29 @@ static const struct drm_format_info *display_get_validated_format(struct drm_dev
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> +	/*
>> +	 * DRM formats assume little-endian byte order. Update the format
>> +	 * if the scanout buffer uses big-endian ordering.
>> +	 */
>> +	if (big_endian) {
>> +		switch (format) {
>> +		case DRM_FORMAT_XRGB8888:
>> +			format = DRM_FORMAT_BGRX8888;
>> +			break;
>> +		case DRM_FORMAT_ARGB8888:
>> +			format = DRM_FORMAT_BGRA8888;
>> +			break;
>> +		case DRM_FORMAT_RGB565:
>> +			format = DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN;
>> +			break;
>> +		case DRM_FORMAT_XRGB1555:
>> +			format = DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>>   	info = drm_format_info(format);
>>   	if (!info) {
>>   		drm_err(dev, "cannot find framebuffer format for depth %u\n", depth);
>> @@ -134,6 +157,23 @@ static int display_read_u32_of(struct drm_device *dev, struct device_node *of_no
>>   	return ret;
>>   }
>>   
>> +static bool display_get_big_endian_of(struct drm_device *dev, struct device_node *of_node)
>> +{
>> +	bool big_endian;
>> +
>> +#ifdef __BIG_ENDIAN
>> +	big_endian = true;
>> +	if (of_get_property(of_node, "little-endian", NULL))
>> +		big_endian = false;
>> +#else
>> +	big_endian = false;
>> +	if (of_get_property(of_node, "big-endian", NULL))
>> +		big_endian = true;
>> +#endif
>> +
>> +	return big_endian;
>> +}
>> +
>>   static int display_get_width_of(struct drm_device *dev, struct device_node *of_node)
>>   {
>>   	u32 width;
>> @@ -613,6 +653,7 @@ static void ofdrm_device_set_gamma_linear(struct ofdrm_device *odev,
>>   
>>   	switch (format->format) {
>>   	case DRM_FORMAT_RGB565:
>> +	case DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN:
>>   		/* Use better interpolation, to take 32 values from 0 to 255 */
>>   		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE / 8; i++) {
>>   			unsigned char r = i * 8 + i / 4;
>> @@ -631,6 +672,7 @@ static void ofdrm_device_set_gamma_linear(struct ofdrm_device *odev,
>>   		}
>>   		break;
>>   	case DRM_FORMAT_XRGB8888:
>> +	case DRM_FORMAT_BGRX8888:
>>   		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE; i++)
>>   			odev->funcs->cmap_write(odev, i, i, i, i);
>>   		break;
>> @@ -650,6 +692,7 @@ static void ofdrm_device_set_gamma(struct ofdrm_device *odev,
>>   
>>   	switch (format->format) {
>>   	case DRM_FORMAT_RGB565:
>> +	case DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN:
>>   		/* Use better interpolation, to take 32 values from lut[0] to lut[255] */
>>   		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE / 8; i++) {
>>   			unsigned char r = lut[i * 8 + i / 4].red >> 8;
>> @@ -668,6 +711,7 @@ static void ofdrm_device_set_gamma(struct ofdrm_device *odev,
>>   		}
>>   		break;
>>   	case DRM_FORMAT_XRGB8888:
>> +	case DRM_FORMAT_BGRX8888:
>>   		for (i = 0; i < OFDRM_GAMMA_LUT_SIZE; i++) {
>>   			unsigned char r = lut[i].red >> 8;
>>   			unsigned char g = lut[i].green >> 8;
>> @@ -718,6 +762,9 @@ static const uint32_t ofdrm_primary_plane_formats[] = {
>>   	DRM_FORMAT_RGB565,
>>   	//DRM_FORMAT_XRGB1555,
>>   	//DRM_FORMAT_C8,
>> +	/* Big-endian formats below */
>> +	DRM_FORMAT_BGRX8888,
>> +	DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN,
>>   };
>>   
>>   static const uint64_t ofdrm_primary_plane_format_modifiers[] = {
>> @@ -1048,6 +1095,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>   	struct ofdrm_device *odev;
>>   	struct drm_device *dev;
>>   	enum ofdrm_model model;
>> +	bool big_endian;
>>   	int width, height, depth, linebytes;
>>   	const struct drm_format_info *format;
>>   	u64 address;
>> @@ -1109,6 +1157,8 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>   		break;
>>   	}
>>   
>> +	big_endian = display_get_big_endian_of(dev, of_node);
>> +
>>   	width = display_get_width_of(dev, of_node);
>>   	if (width < 0)
>>   		return ERR_PTR(width);
>> @@ -1122,7 +1172,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>   	if (linebytes < 0)
>>   		return ERR_PTR(linebytes);
>>   
>> -	format = display_get_validated_format(dev, depth);
>> +	format = display_get_validated_format(dev, depth, big_endian);
>>   	if (IS_ERR(format))
>>   		return ERR_CAST(format);
>>   	if (!linebytes) {
>> @@ -1234,6 +1284,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>   		dev->mode_config.preferred_depth = depth;
>>   		break;
>>   	}
>> +	dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>>   
>>   	/* Primary plane */
>>   
>> -- 
>> 2.37.3
>>