diff mbox series

[v12,2/9] drm/ingenic: Add support for JZ4780 and HDMI output

Message ID 6a7b188769a7ad477bf8cb71e1b9bc086b92388d.1643632014.git.hns@goldelico.com (mailing list archive)
State Superseded
Headers show
Series MIPS: JZ4780 and CI20 HDMI | expand

Commit Message

H. Nikolaus Schaller Jan. 31, 2022, 12:26 p.m. UTC
From: Paul Boddie <paul@boddie.org.uk>

Add support for the LCD controller present on JZ4780 SoCs.
This SoC uses 8-byte descriptors which extend the current
4-byte descriptors used for other Ingenic SoCs.

Tested on MIPS Creator CI20 board.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 ++++++++++++++++++++++-
 drivers/gpu/drm/ingenic/ingenic-drm.h     | 38 ++++++++++++++
 2 files changed, 98 insertions(+), 1 deletion(-)

Comments

Paul Cercueil Feb. 2, 2022, 10:23 a.m. UTC | #1
Hi Nikolaus,

Le lun., janv. 31 2022 at 13:26:48 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Paul Boddie <paul@boddie.org.uk>
> 
> Add support for the LCD controller present on JZ4780 SoCs.
> This SoC uses 8-byte descriptors which extend the current
> 4-byte descriptors used for other Ingenic SoCs.
> 
> Tested on MIPS Creator CI20 board.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 
> ++++++++++++++++++++++-
>  drivers/gpu/drm/ingenic/ingenic-drm.h     | 38 ++++++++++++++
>  2 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 9c60fc4605e4b..ccdb9eedd9247 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -6,6 +6,7 @@
> 
>  #include "ingenic-drm.h"
> 
> +#include <linux/bitfield.h>
>  #include <linux/component.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
> @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc {
>  	u32 addr;
>  	u32 id;
>  	u32 cmd;
> +	/* extended hw descriptor for jz4780 */
> +	u32 offsize;
> +	u32 pagewidth;
> +	u32 cpos;
> +	u32 dessize;
>  } __aligned(16);
> 
>  struct ingenic_dma_hwdescs {
> @@ -59,7 +65,9 @@ struct ingenic_dma_hwdescs {
>  struct jz_soc_info {
>  	bool needs_dev_clk;
>  	bool has_osd;
> +	bool has_alpha;
>  	bool map_noncoherent;
> +	bool use_extended_hwdesc;
>  	unsigned int max_width, max_height;
>  	const u32 *formats_f0, *formats_f1;
>  	unsigned int num_formats_f0, num_formats_f1;
> @@ -446,6 +454,9 @@ static int ingenic_drm_plane_atomic_check(struct 
> drm_plane *plane,
>  	if (!crtc)
>  		return 0;
> 
> +	if (plane == &priv->f0)
> +		return -EINVAL;

This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly 
usable there.

Cheers,
-Paul

> +
>  	crtc_state = drm_atomic_get_existing_crtc_state(state,
>  							crtc);
>  	if (WARN_ON(!crtc_state))
> @@ -662,6 +673,33 @@ static void 
> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
>  		hwdesc->next = dma_hwdesc_addr(priv, next_id);
> 
> +		if (priv->soc_info->use_extended_hwdesc) {
> +			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
> +
> +			/* Extended 8-byte descriptor */
> +			hwdesc->cpos = 0;
> +			hwdesc->offsize = 0;
> +			hwdesc->pagewidth = 0;
> +
> +			switch (newstate->fb->format->format) {
> +			case DRM_FORMAT_XRGB1555:
> +				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
> +				fallthrough;
> +			case DRM_FORMAT_RGB565:
> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
> +				break;
> +			case DRM_FORMAT_XRGB8888:
> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
> +				break;
> +			}
> +			hwdesc->cpos |= (JZ_LCD_CPOS_COEFFICIENT_1 <<
> +					 JZ_LCD_CPOS_COEFFICIENT_OFFSET);
> +			hwdesc->dessize =
> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
> +				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK, height - 1) |
> +				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK, width - 1);
> +		}
> +
>  		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  			fourcc = newstate->fb->format->format;
> 
> @@ -693,6 +731,9 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>  	}
> 
> +	if (priv->soc_info->use_extended_hwdesc)
> +		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
> +
>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>  		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> @@ -1015,6 +1056,7 @@ static int ingenic_drm_bind(struct device *dev, 
> bool has_components)
>  	long parent_rate;
>  	unsigned int i, clone_mask = 0;
>  	int ret, irq;
> +	u32 osdc = 0;
> 
>  	soc_info = of_device_get_match_data(dev);
>  	if (!soc_info) {
> @@ -1272,7 +1314,10 @@ static int ingenic_drm_bind(struct device 
> *dev, bool has_components)
> 
>  	/* Enable OSD if available */
>  	if (soc_info->has_osd)
> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
> +		osdc |= JZ_LCD_OSDC_OSDEN;
> +	if (soc_info->has_alpha)
> +		osdc |= JZ_LCD_OSDC_ALPHAEN;
> +	regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc);
> 
>  	mutex_init(&priv->clk_mutex);
>  	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
> @@ -1468,10 +1513,24 @@ static const struct jz_soc_info 
> jz4770_soc_info = {
>  	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>  };
> 
> +static const struct jz_soc_info jz4780_soc_info = {
> +	.needs_dev_clk = true,
> +	.has_osd = true,
> +	.has_alpha = true,
> +	.use_extended_hwdesc = true,
> +	.max_width = 4096,
> +	.max_height = 2048,
> +	.formats_f1 = jz4770_formats_f1,
> +	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
> +	.formats_f0 = jz4770_formats_f0,
> +	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
> +};
> +
>  static const struct of_device_id ingenic_drm_of_match[] = {
>  	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
>  	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
>  	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
> +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h 
> b/drivers/gpu/drm/ingenic/ingenic-drm.h
> index 22654ac1dde1c..cb1d09b625881 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
> @@ -44,8 +44,11 @@
>  #define JZ_REG_LCD_XYP1				0x124
>  #define JZ_REG_LCD_SIZE0			0x128
>  #define JZ_REG_LCD_SIZE1			0x12c
> +#define JZ_REG_LCD_PCFG				0x2c0
> 
>  #define JZ_LCD_CFG_SLCD				BIT(31)
> +#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
> +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
>  #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
>  #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
>  #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
> @@ -63,6 +66,7 @@
>  #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
>  #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
>  #define JZ_LCD_CFG_18_BIT			BIT(7)
> +#define JZ_LCD_CFG_24_BIT			BIT(6)
>  #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
> 
>  #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
> @@ -132,6 +136,7 @@
>  #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
>  #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
>  #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
> +#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
> 
>  #define JZ_LCD_SYNC_MASK			0x3ff
> 
> @@ -153,6 +158,7 @@
>  #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
> 
>  #define JZ_LCD_OSDC_OSDEN			BIT(0)
> +#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
>  #define JZ_LCD_OSDC_F0EN			BIT(3)
>  #define JZ_LCD_OSDC_F1EN			BIT(4)
> 
> @@ -176,6 +182,38 @@
>  #define JZ_LCD_SIZE01_WIDTH_LSB			0
>  #define JZ_LCD_SIZE01_HEIGHT_LSB		16
> 
> +#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
> +#define JZ_LCD_DESSIZE_HEIGHT_MASK		GENMASK(23, 12)
> +#define JZ_LCD_DESSIZE_WIDTH_MASK		GENMASK(11, 0)
> +
> +#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
> +#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
> +#define JZ_LCD_CPOS_BPP_30			(7 << 27)
> +#define JZ_LCD_CPOS_RGB555			BIT(30)
> +#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
> +#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
> +#define JZ_LCD_CPOS_COEFFICIENT_0		0
> +#define JZ_LCD_CPOS_COEFFICIENT_1		1
> +#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1		2
> +#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1	3
> +
> +#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
> +#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
> +#define JZ_LCD_RGBC_422				BIT(8)
> +#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
> +
> +#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
> +#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
> +#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
> +#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
> +#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
> +#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
> +#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
> +#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
> +#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
> +#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
> +#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
> +
>  struct device;
>  struct drm_plane;
>  struct drm_plane_state;
> --
> 2.33.0
>
H. Nikolaus Schaller Feb. 2, 2022, 11:56 a.m. UTC | #2
Hi Paul,
thanks for the reviews. Looks as if we are close to making a goal.

> Am 02.02.2022 um 11:23 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le lun., janv. 31 2022 at 13:26:48 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> From: Paul Boddie <paul@boddie.org.uk>
>> Add support for the LCD controller present on JZ4780 SoCs.
>> This SoC uses 8-byte descriptors which extend the current
>> 4-byte descriptors used for other Ingenic SoCs.
>> Tested on MIPS Creator CI20 board.
>> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 ++++++++++++++++++++++-
>> drivers/gpu/drm/ingenic/ingenic-drm.h     | 38 ++++++++++++++
>> 2 files changed, 98 insertions(+), 1 deletion(-)
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index 9c60fc4605e4b..ccdb9eedd9247 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -6,6 +6,7 @@
>> #include "ingenic-drm.h"
>> +#include <linux/bitfield.h>
>> #include <linux/component.h>
>> #include <linux/clk.h>
>> #include <linux/dma-mapping.h>
>> @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc {
>> 	u32 addr;
>> 	u32 id;
>> 	u32 cmd;
>> +	/* extended hw descriptor for jz4780 */
>> +	u32 offsize;
>> +	u32 pagewidth;
>> +	u32 cpos;
>> +	u32 dessize;
>> } __aligned(16);
>> struct ingenic_dma_hwdescs {
>> @@ -59,7 +65,9 @@ struct ingenic_dma_hwdescs {
>> struct jz_soc_info {
>> 	bool needs_dev_clk;
>> 	bool has_osd;
>> +	bool has_alpha;
>> 	bool map_noncoherent;
>> +	bool use_extended_hwdesc;
>> 	unsigned int max_width, max_height;
>> 	const u32 *formats_f0, *formats_f1;
>> 	unsigned int num_formats_f0, num_formats_f1;
>> @@ -446,6 +454,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>> 	if (!crtc)
>> 		return 0;
>> +	if (plane == &priv->f0)
>> +		return -EINVAL;
> 
> This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly usable there.

Hm. I think it was your request/proposal to add this [1]?

What I have forgotten is why the f0 plane should not be usable for jz4780.

BR and thanks,
Nikolaus

[1] end of https://patchwork.kernel.org/project/dri-devel/patch/2c7d0aa7d3ef480ebb996d37c27cbaa6f722728b.1633436959.git.hns@goldelico.com/#24578683


> 
> Cheers,
> -Paul
> 
>> +
>> 	crtc_state = drm_atomic_get_existing_crtc_state(state,
>> 							crtc);
>> 	if (WARN_ON(!crtc_state))
>> @@ -662,6 +673,33 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>> 		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
>> 		hwdesc->next = dma_hwdesc_addr(priv, next_id);
>> +		if (priv->soc_info->use_extended_hwdesc) {
>> +			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
>> +
>> +			/* Extended 8-byte descriptor */
>> +			hwdesc->cpos = 0;
>> +			hwdesc->offsize = 0;
>> +			hwdesc->pagewidth = 0;
>> +
>> +			switch (newstate->fb->format->format) {
>> +			case DRM_FORMAT_XRGB1555:
>> +				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
>> +				fallthrough;
>> +			case DRM_FORMAT_RGB565:
>> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
>> +				break;
>> +			case DRM_FORMAT_XRGB8888:
>> +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
>> +				break;
>> +			}
>> +			hwdesc->cpos |= (JZ_LCD_CPOS_COEFFICIENT_1 <<
>> +					 JZ_LCD_CPOS_COEFFICIENT_OFFSET);
>> +			hwdesc->dessize =
>> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
>> +				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK, height - 1) |
>> +				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK, width - 1);
>> +		}
>> +
>> 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> 			fourcc = newstate->fb->format->format;
>> @@ -693,6 +731,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> 	}
>> +	if (priv->soc_info->use_extended_hwdesc)
>> +		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
>> +
>> 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>> 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>> 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>> @@ -1015,6 +1056,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	long parent_rate;
>> 	unsigned int i, clone_mask = 0;
>> 	int ret, irq;
>> +	u32 osdc = 0;
>> 	soc_info = of_device_get_match_data(dev);
>> 	if (!soc_info) {
>> @@ -1272,7 +1314,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	/* Enable OSD if available */
>> 	if (soc_info->has_osd)
>> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +		osdc |= JZ_LCD_OSDC_OSDEN;
>> +	if (soc_info->has_alpha)
>> +		osdc |= JZ_LCD_OSDC_ALPHAEN;
>> +	regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc);
>> 	mutex_init(&priv->clk_mutex);
>> 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1468,10 +1513,24 @@ static const struct jz_soc_info jz4770_soc_info = {
>> 	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>> };
>> +static const struct jz_soc_info jz4780_soc_info = {
>> +	.needs_dev_clk = true,
>> +	.has_osd = true,
>> +	.has_alpha = true,
>> +	.use_extended_hwdesc = true,
>> +	.max_width = 4096,
>> +	.max_height = 2048,
>> +	.formats_f1 = jz4770_formats_f1,
>> +	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>> +	.formats_f0 = jz4770_formats_f0,
>> +	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>> +};
>> +
>> static const struct of_device_id ingenic_drm_of_match[] = {
>> 	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
>> 	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
>> 	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
>> +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
>> 	{ /* sentinel */ },
>> };
>> MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
>> index 22654ac1dde1c..cb1d09b625881 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
>> @@ -44,8 +44,11 @@
>> #define JZ_REG_LCD_XYP1				0x124
>> #define JZ_REG_LCD_SIZE0			0x128
>> #define JZ_REG_LCD_SIZE1			0x12c
>> +#define JZ_REG_LCD_PCFG				0x2c0
>> #define JZ_LCD_CFG_SLCD				BIT(31)
>> +#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
>> +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
>> #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
>> #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
>> #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
>> @@ -63,6 +66,7 @@
>> #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
>> #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
>> #define JZ_LCD_CFG_18_BIT			BIT(7)
>> +#define JZ_LCD_CFG_24_BIT			BIT(6)
>> #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
>> #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
>> @@ -132,6 +136,7 @@
>> #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
>> #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
>> #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
>> +#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
>> #define JZ_LCD_SYNC_MASK			0x3ff
>> @@ -153,6 +158,7 @@
>> #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
>> #define JZ_LCD_OSDC_OSDEN			BIT(0)
>> +#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
>> #define JZ_LCD_OSDC_F0EN			BIT(3)
>> #define JZ_LCD_OSDC_F1EN			BIT(4)
>> @@ -176,6 +182,38 @@
>> #define JZ_LCD_SIZE01_WIDTH_LSB			0
>> #define JZ_LCD_SIZE01_HEIGHT_LSB		16
>> +#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
>> +#define JZ_LCD_DESSIZE_HEIGHT_MASK		GENMASK(23, 12)
>> +#define JZ_LCD_DESSIZE_WIDTH_MASK		GENMASK(11, 0)
>> +
>> +#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
>> +#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
>> +#define JZ_LCD_CPOS_BPP_30			(7 << 27)
>> +#define JZ_LCD_CPOS_RGB555			BIT(30)
>> +#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
>> +#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
>> +#define JZ_LCD_CPOS_COEFFICIENT_0		0
>> +#define JZ_LCD_CPOS_COEFFICIENT_1		1
>> +#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1		2
>> +#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1	3
>> +
>> +#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
>> +#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
>> +#define JZ_LCD_RGBC_422				BIT(8)
>> +#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
>> +
>> +#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
>> +#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
>> +#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
>> +#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
>> +#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
>> +#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
>> +#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
>> +#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
>> +#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
>> +#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
>> +#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
>> +
>> struct device;
>> struct drm_plane;
>> struct drm_plane_state;
>> --
>> 2.33.0
> 
>
Paul Cercueil Feb. 2, 2022, 12:06 p.m. UTC | #3
Hi Nikolaus,

Le mer., févr. 2 2022 at 12:56:35 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> thanks for the reviews. Looks as if we are close to making a goal.
> 
>>  Am 02.02.2022 um 11:23 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi Nikolaus,
>> 
>>  Le lun., janv. 31 2022 at 13:26:48 +0100, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  From: Paul Boddie <paul@boddie.org.uk>
>>>  Add support for the LCD controller present on JZ4780 SoCs.
>>>  This SoC uses 8-byte descriptors which extend the current
>>>  4-byte descriptors used for other Ingenic SoCs.
>>>  Tested on MIPS Creator CI20 board.
>>>  Signed-off-by: Paul Boddie <paul@boddie.org.uk>
>>>  Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>>  Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>  ---
>>>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 61 
>>> ++++++++++++++++++++++-
>>>  drivers/gpu/drm/ingenic/ingenic-drm.h     | 38 ++++++++++++++
>>>  2 files changed, 98 insertions(+), 1 deletion(-)
>>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>  index 9c60fc4605e4b..ccdb9eedd9247 100644
>>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>  @@ -6,6 +6,7 @@
>>>  #include "ingenic-drm.h"
>>>  +#include <linux/bitfield.h>
>>>  #include <linux/component.h>
>>>  #include <linux/clk.h>
>>>  #include <linux/dma-mapping.h>
>>>  @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc {
>>>  	u32 addr;
>>>  	u32 id;
>>>  	u32 cmd;
>>>  +	/* extended hw descriptor for jz4780 */
>>>  +	u32 offsize;
>>>  +	u32 pagewidth;
>>>  +	u32 cpos;
>>>  +	u32 dessize;
>>>  } __aligned(16);
>>>  struct ingenic_dma_hwdescs {
>>>  @@ -59,7 +65,9 @@ struct ingenic_dma_hwdescs {
>>>  struct jz_soc_info {
>>>  	bool needs_dev_clk;
>>>  	bool has_osd;
>>>  +	bool has_alpha;
>>>  	bool map_noncoherent;
>>>  +	bool use_extended_hwdesc;
>>>  	unsigned int max_width, max_height;
>>>  	const u32 *formats_f0, *formats_f1;
>>>  	unsigned int num_formats_f0, num_formats_f1;
>>>  @@ -446,6 +454,9 @@ static int 
>>> ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>  	if (!crtc)
>>>  		return 0;
>>>  +	if (plane == &priv->f0)
>>>  +		return -EINVAL;
>> 
>>  This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly 
>> usable there.
> 
> Hm. I think it was your request/proposal to add this [1]?

Because otherwise with your current patchset the f0 plane does not work 
*on JZ4780*. It does work on older SoCs.

> What I have forgotten is why the f0 plane should not be usable for 
> jz4780.

We return an error here to prevent userspace from using the f0 plane 
until it's effectively working on the JZ4780.

Cheers,
-Paul

> BR and thanks,
> Nikolaus
> 
> [1] end of 
> https://patchwork.kernel.org/project/dri-devel/patch/2c7d0aa7d3ef480ebb996d37c27cbaa6f722728b.1633436959.git.hns@goldelico.com/#24578683
> 
> 
>> 
>>  Cheers,
>>  -Paul
>> 
>>>  +
>>>  	crtc_state = drm_atomic_get_existing_crtc_state(state,
>>>  							crtc);
>>>  	if (WARN_ON(!crtc_state))
>>>  @@ -662,6 +673,33 @@ static void 
>>> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>>>  		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
>>>  		hwdesc->next = dma_hwdesc_addr(priv, next_id);
>>>  +		if (priv->soc_info->use_extended_hwdesc) {
>>>  +			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
>>>  +
>>>  +			/* Extended 8-byte descriptor */
>>>  +			hwdesc->cpos = 0;
>>>  +			hwdesc->offsize = 0;
>>>  +			hwdesc->pagewidth = 0;
>>>  +
>>>  +			switch (newstate->fb->format->format) {
>>>  +			case DRM_FORMAT_XRGB1555:
>>>  +				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
>>>  +				fallthrough;
>>>  +			case DRM_FORMAT_RGB565:
>>>  +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
>>>  +				break;
>>>  +			case DRM_FORMAT_XRGB8888:
>>>  +				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
>>>  +				break;
>>>  +			}
>>>  +			hwdesc->cpos |= (JZ_LCD_CPOS_COEFFICIENT_1 <<
>>>  +					 JZ_LCD_CPOS_COEFFICIENT_OFFSET);
>>>  +			hwdesc->dessize =
>>>  +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
>>>  +				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK, height - 1) |
>>>  +				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK, width - 1);
>>>  +		}
>>>  +
>>>  		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>>>  			fourcc = newstate->fb->format->format;
>>>  @@ -693,6 +731,9 @@ static void 
>>> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>>>  		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>>>  	}
>>>  +	if (priv->soc_info->use_extended_hwdesc)
>>>  +		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
>>>  +
>>>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>>>  		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>>>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>>>  @@ -1015,6 +1056,7 @@ static int ingenic_drm_bind(struct device 
>>> *dev, bool has_components)
>>>  	long parent_rate;
>>>  	unsigned int i, clone_mask = 0;
>>>  	int ret, irq;
>>>  +	u32 osdc = 0;
>>>  	soc_info = of_device_get_match_data(dev);
>>>  	if (!soc_info) {
>>>  @@ -1272,7 +1314,10 @@ static int ingenic_drm_bind(struct device 
>>> *dev, bool has_components)
>>>  	/* Enable OSD if available */
>>>  	if (soc_info->has_osd)
>>>  -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>>>  +		osdc |= JZ_LCD_OSDC_OSDEN;
>>>  +	if (soc_info->has_alpha)
>>>  +		osdc |= JZ_LCD_OSDC_ALPHAEN;
>>>  +	regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc);
>>>  	mutex_init(&priv->clk_mutex);
>>>  	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>>>  @@ -1468,10 +1513,24 @@ static const struct jz_soc_info 
>>> jz4770_soc_info = {
>>>  	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>>>  };
>>>  +static const struct jz_soc_info jz4780_soc_info = {
>>>  +	.needs_dev_clk = true,
>>>  +	.has_osd = true,
>>>  +	.has_alpha = true,
>>>  +	.use_extended_hwdesc = true,
>>>  +	.max_width = 4096,
>>>  +	.max_height = 2048,
>>>  +	.formats_f1 = jz4770_formats_f1,
>>>  +	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>>>  +	.formats_f0 = jz4770_formats_f0,
>>>  +	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>>>  +};
>>>  +
>>>  static const struct of_device_id ingenic_drm_of_match[] = {
>>>  	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
>>>  	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info 
>>> },
>>>  	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
>>>  +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
>>>  	{ /* sentinel */ },
>>>  };
>>>  MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
>>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h 
>>> b/drivers/gpu/drm/ingenic/ingenic-drm.h
>>>  index 22654ac1dde1c..cb1d09b625881 100644
>>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
>>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
>>>  @@ -44,8 +44,11 @@
>>>  #define JZ_REG_LCD_XYP1				0x124
>>>  #define JZ_REG_LCD_SIZE0			0x128
>>>  #define JZ_REG_LCD_SIZE1			0x12c
>>>  +#define JZ_REG_LCD_PCFG				0x2c0
>>>  #define JZ_LCD_CFG_SLCD				BIT(31)
>>>  +#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
>>>  +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
>>>  #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
>>>  #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
>>>  #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
>>>  @@ -63,6 +66,7 @@
>>>  #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
>>>  #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
>>>  #define JZ_LCD_CFG_18_BIT			BIT(7)
>>>  +#define JZ_LCD_CFG_24_BIT			BIT(6)
>>>  #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
>>>  #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
>>>  @@ -132,6 +136,7 @@
>>>  #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
>>>  #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
>>>  #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
>>>  +#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
>>>  #define JZ_LCD_SYNC_MASK			0x3ff
>>>  @@ -153,6 +158,7 @@
>>>  #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
>>>  #define JZ_LCD_OSDC_OSDEN			BIT(0)
>>>  +#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
>>>  #define JZ_LCD_OSDC_F0EN			BIT(3)
>>>  #define JZ_LCD_OSDC_F1EN			BIT(4)
>>>  @@ -176,6 +182,38 @@
>>>  #define JZ_LCD_SIZE01_WIDTH_LSB			0
>>>  #define JZ_LCD_SIZE01_HEIGHT_LSB		16
>>>  +#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
>>>  +#define JZ_LCD_DESSIZE_HEIGHT_MASK		GENMASK(23, 12)
>>>  +#define JZ_LCD_DESSIZE_WIDTH_MASK		GENMASK(11, 0)
>>>  +
>>>  +#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
>>>  +#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
>>>  +#define JZ_LCD_CPOS_BPP_30			(7 << 27)
>>>  +#define JZ_LCD_CPOS_RGB555			BIT(30)
>>>  +#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
>>>  +#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
>>>  +#define JZ_LCD_CPOS_COEFFICIENT_0		0
>>>  +#define JZ_LCD_CPOS_COEFFICIENT_1		1
>>>  +#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1		2
>>>  +#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1	3
>>>  +
>>>  +#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
>>>  +#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
>>>  +#define JZ_LCD_RGBC_422				BIT(8)
>>>  +#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
>>>  +
>>>  +#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
>>>  +#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
>>>  +#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
>>>  +#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
>>>  +#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
>>>  +#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
>>>  +#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
>>>  +#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
>>>  +#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
>>>  +#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
>>>  +#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
>>>  +
>>>  struct device;
>>>  struct drm_plane;
>>>  struct drm_plane_state;
>>>  --
>>>  2.33.0
>> 
>> 
>
H. Nikolaus Schaller Feb. 2, 2022, 12:17 p.m. UTC | #4
Hi Paul,

> Am 02.02.2022 um 13:06 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
>>>> @@ -446,6 +454,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>> 	if (!crtc)
>>>> 		return 0;
>>>> +	if (plane == &priv->f0)
>>>> +		return -EINVAL;
>>> This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly usable there.
>> Hm. I think it was your request/proposal to add this [1]?
> 
> Because otherwise with your current patchset the f0 plane does not work *on JZ4780*.

Not that I am eager to fix that, but...
maybe it could be better to fix than having the check and -EINVAL depend on SoC compatible string
(or some new flag in soc_info. plane_f0_not_working)?

> It does work on older SoCs.
> 
>> What I have forgotten is why the f0 plane should not be usable for jz4780.
> 
> We return an error here to prevent userspace from using the f0 plane until it's effectively working on the JZ4780.

Well, what would be not working with that plane if user-space would try to use it?

> 
> Cheers,
> -Paul

BR and thanks,
Nikolaus
Paul Cercueil Feb. 2, 2022, 12:28 p.m. UTC | #5
Le mer., févr. 2 2022 at 13:17:14 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 02.02.2022 um 13:06 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi Nikolaus,
>> 
>>>>>  @@ -446,6 +454,9 @@ static int 
>>>>> ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>>>  	if (!crtc)
>>>>>  		return 0;
>>>>>  +	if (plane == &priv->f0)
>>>>>  +		return -EINVAL;
>>>>  This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly 
>>>> usable there.
>>>  Hm. I think it was your request/proposal to add this [1]?
>> 
>>  Because otherwise with your current patchset the f0 plane does not 
>> work *on JZ4780*.
> 
> Not that I am eager to fix that, but...
> maybe it could be better to fix than having the check and -EINVAL 
> depend on SoC compatible string
> (or some new flag in soc_info. plane_f0_not_working)?

Totally agree! A proper fix would be much better. A 
"plane_f0_not_working" in the meantime is OK with me.

Note that there are other things not working with your current 
implementation, for instance you cannot set the X/Y start position of 
the f1 plane, which means it's only really usable for fullscreen 
desktop/windows.

>>  It does work on older SoCs.
>> 
>>>  What I have forgotten is why the f0 plane should not be usable for 
>>> jz4780.
>> 
>>  We return an error here to prevent userspace from using the f0 
>> plane until it's effectively working on the JZ4780.
> 
> Well, what would be not working with that plane if user-space would 
> try to use it?

 From what I remember, it wouldn't show anything on screen, and after 
that trying to use the f1 plane wouldn't work either.

-Paul
H. Nikolaus Schaller Feb. 2, 2022, 12:33 p.m. UTC | #6
> Am 02.02.2022 um 13:28 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le mer., févr. 2 2022 at 13:17:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 02.02.2022 um 13:06 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Hi Nikolaus,
>>>>>> @@ -446,6 +454,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>>>> 	if (!crtc)
>>>>>> 		return 0;
>>>>>> +	if (plane == &priv->f0)
>>>>>> +		return -EINVAL;
>>>>> This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly usable there.
>>>> Hm. I think it was your request/proposal to add this [1]?
>>> Because otherwise with your current patchset the f0 plane does not work *on JZ4780*.
>> Not that I am eager to fix that, but...
>> maybe it could be better to fix than having the check and -EINVAL depend on SoC compatible string
>> (or some new flag in soc_info. plane_f0_not_working)?
> 
> Totally agree! A proper fix would be much better. A "plane_f0_not_working" in the meantime is OK with me.

Ok, then I'll prepare a v13 with plane_f0_not_working.

> 
> Note that there are other things not working with your current implementation, for instance you cannot set the X/Y start position of the f1 plane, which means it's only really usable for fullscreen desktop/windows.

Is setting x/y possible for the other SoC?

> 
>>> It does work on older SoCs.
>>>> What I have forgotten is why the f0 plane should not be usable for jz4780.
>>> We return an error here to prevent userspace from using the f0 plane until it's effectively working on the JZ4780.
>> Well, what would be not working with that plane if user-space would try to use it?
> 
> From what I remember, it wouldn't show anything on screen, and after that trying to use the f1 plane wouldn't work either.

Ok. That may become a big project to fix. So let's do step 1 first.

BR and thanks,
NIkolaus
Paul Cercueil Feb. 2, 2022, 12:41 p.m. UTC | #7
Le mer., févr. 2 2022 at 13:33:15 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> 
> 
>>  Am 02.02.2022 um 13:28 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> 
>> 
>>  Le mer., févr. 2 2022 at 13:17:14 +0100, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  Hi Paul,
>>>>  Am 02.02.2022 um 13:06 schrieb Paul Cercueil 
>>>> <paul@crapouillou.net>:
>>>>  Hi Nikolaus,
>>>>>>>  @@ -446,6 +454,9 @@ static int 
>>>>>>> ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>>>>>  	if (!crtc)
>>>>>>>  		return 0;
>>>>>>>  +	if (plane == &priv->f0)
>>>>>>>  +		return -EINVAL;
>>>>>>  This will break JZ4725B -> JZ4770 SoCs, the f0 plane is 
>>>>>> perfectly usable there.
>>>>>  Hm. I think it was your request/proposal to add this [1]?
>>>>  Because otherwise with your current patchset the f0 plane does 
>>>> not work *on JZ4780*.
>>>  Not that I am eager to fix that, but...
>>>  maybe it could be better to fix than having the check and -EINVAL 
>>> depend on SoC compatible string
>>>  (or some new flag in soc_info. plane_f0_not_working)?
>> 
>>  Totally agree! A proper fix would be much better. A 
>> "plane_f0_not_working" in the meantime is OK with me.
> 
> Ok, then I'll prepare a v13 with plane_f0_not_working.
> 
>> 
>>  Note that there are other things not working with your current 
>> implementation, for instance you cannot set the X/Y start position 
>> of the f1 plane, which means it's only really usable for fullscreen 
>> desktop/windows.
> 
> Is setting x/y possible for the other SoC?

Yes. They support different x/y positions, sizes, and pixel format for 
both f0, f1 and IPU planes.

-Paul

>> 
>>>>  It does work on older SoCs.
>>>>>  What I have forgotten is why the f0 plane should not be usable 
>>>>> for jz4780.
>>>>  We return an error here to prevent userspace from using the f0 
>>>> plane until it's effectively working on the JZ4780.
>>>  Well, what would be not working with that plane if user-space 
>>> would try to use it?
>> 
>>  From what I remember, it wouldn't show anything on screen, and 
>> after that trying to use the f1 plane wouldn't work either.
> 
> Ok. That may become a big project to fix. So let's do step 1 first.
> 
> BR and thanks,
> NIkolaus
>
H. Nikolaus Schaller Feb. 2, 2022, 12:48 p.m. UTC | #8
> Am 02.02.2022 um 13:41 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le mer., févr. 2 2022 at 13:33:15 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>> Am 02.02.2022 um 13:28 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Le mer., févr. 2 2022 at 13:17:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>> Hi Paul,
>>>>> Am 02.02.2022 um 13:06 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>>> Hi Nikolaus,
>>>>>>>> @@ -446,6 +454,9 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>>>>>> 	if (!crtc)
>>>>>>>> 		return 0;
>>>>>>>> +	if (plane == &priv->f0)
>>>>>>>> +		return -EINVAL;
>>>>>>> This will break JZ4725B -> JZ4770 SoCs, the f0 plane is perfectly usable there.
>>>>>> Hm. I think it was your request/proposal to add this [1]?
>>>>> Because otherwise with your current patchset the f0 plane does not work *on JZ4780*.
>>>> Not that I am eager to fix that, but...
>>>> maybe it could be better to fix than having the check and -EINVAL depend on SoC compatible string
>>>> (or some new flag in soc_info. plane_f0_not_working)?
>>> Totally agree! A proper fix would be much better. A "plane_f0_not_working" in the meantime is OK with me.
>> Ok, then I'll prepare a v13 with plane_f0_not_working.
>>> Note that there are other things not working with your current implementation, for instance you cannot set the X/Y start position of the f1 plane, which means it's only really usable for fullscreen desktop/windows.
>> Is setting x/y possible for the other SoC?
> 
> Yes. They support different x/y positions, sizes, and pixel format for both f0, f1 and IPU planes.

Hm. What I don't get is why the jz4780 doesn't support that equally well with existing code?
To me it looks mainly like an extended jz4740. But I have to admit that I did not study this deeply.

I am happy with a working desktop HDMI setup...

BR,
Nikolaus
Paul Boddie Feb. 2, 2022, 5:04 p.m. UTC | #9
On Wednesday, 2 February 2022 13:41:21 CET Paul Cercueil wrote:
> Le mer., févr. 2 2022 at 13:33:15 +0100, H. Nikolaus Schaller> 
<hns@goldelico.com> a écrit :
> >>  Am 02.02.2022 um 13:28 schrieb Paul Cercueil <paul@crapouillou.net>:
> >>  
> >>  Note that there are other things not working with your current
> >> 
> >> implementation, for instance you cannot set the X/Y start position
> >> of the f1 plane, which means it's only really usable for fullscreen
> >> desktop/windows.
> > 
> > Is setting x/y possible for the other SoC?
> 
> Yes. They support different x/y positions, sizes, and pixel format for
> both f0, f1 and IPU planes.

One thing worth noting about the JZ4780 is that a lot of the registers that 
might otherwise be used for the above purposes appear to be read-only, at 
least for the different fields concerned.

For example, those affecting ingenic_drm_plane_config:

Control Register (LCDCTRL) - specifically the BPP0 field
OSD Control Register (LCDOSDCTRL)
Foreground 0 XY Position Register (LCDXYP0)
Foreground 1 XY Position Register (LCDXYP1)
Foreground 0 Size Register (LCDSIZE0)
Foreground 1 Size Register (LCDSIZE1)

These require changes to the extended descriptor members instead, and I am 
fairly sure I mentioned the implications for pixel depth configuration 
previously. So, as far as I can tell, we would need to update the descriptors, 
not the registers, to support the operations mentioned above.

As for the f0 plane "not working", I am not aware of any limitation around 
using only f0 (assuming it corresponds to what the manual calls fg0) or only 
f1 (again, assuming fg1 in the manual) or both. My assumption was that for 
this particular driver, f0 was reserved for some kind of overlay and that f1 
was to be used for the normal non-overlay display for products where the OSD 
peripheral is provided.

From the definition of struct ingenic_drm:

        /*
         * f1 (aka. foreground1) is our primary plane, on top of which
         * f0 (aka. foreground0) can be overlayed. Z-order is fixed in
         * hardware and cannot be changed.
         */

So, as I understood it, the driver would configure f1 in the case of the 
JZ4780 for basic display support. Configuring f0 as an overlay should be 
entirely possible, but I imagine that it needs to change the descriptors, not 
the registers, to have a chance of actually working.

I hope this is somewhat useful information. I honestly don't know if, say, the 
JZ4770 has a similar arrangement with regard to configuration via descriptors, 
as opposed to registers, but I think it is an important distinction between 
devices in this particular family that needs to be accommodated in the driver, 
and we obviously want to determine how this might best be achieved.

Paul
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 9c60fc4605e4b..ccdb9eedd9247 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -6,6 +6,7 @@ 
 
 #include "ingenic-drm.h"
 
+#include <linux/bitfield.h>
 #include <linux/component.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
@@ -49,6 +50,11 @@  struct ingenic_dma_hwdesc {
 	u32 addr;
 	u32 id;
 	u32 cmd;
+	/* extended hw descriptor for jz4780 */
+	u32 offsize;
+	u32 pagewidth;
+	u32 cpos;
+	u32 dessize;
 } __aligned(16);
 
 struct ingenic_dma_hwdescs {
@@ -59,7 +65,9 @@  struct ingenic_dma_hwdescs {
 struct jz_soc_info {
 	bool needs_dev_clk;
 	bool has_osd;
+	bool has_alpha;
 	bool map_noncoherent;
+	bool use_extended_hwdesc;
 	unsigned int max_width, max_height;
 	const u32 *formats_f0, *formats_f1;
 	unsigned int num_formats_f0, num_formats_f1;
@@ -446,6 +454,9 @@  static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 	if (!crtc)
 		return 0;
 
+	if (plane == &priv->f0)
+		return -EINVAL;
+
 	crtc_state = drm_atomic_get_existing_crtc_state(state,
 							crtc);
 	if (WARN_ON(!crtc_state))
@@ -662,6 +673,33 @@  static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
 		hwdesc->next = dma_hwdesc_addr(priv, next_id);
 
+		if (priv->soc_info->use_extended_hwdesc) {
+			hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE;
+
+			/* Extended 8-byte descriptor */
+			hwdesc->cpos = 0;
+			hwdesc->offsize = 0;
+			hwdesc->pagewidth = 0;
+
+			switch (newstate->fb->format->format) {
+			case DRM_FORMAT_XRGB1555:
+				hwdesc->cpos |= JZ_LCD_CPOS_RGB555;
+				fallthrough;
+			case DRM_FORMAT_RGB565:
+				hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16;
+				break;
+			case DRM_FORMAT_XRGB8888:
+				hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
+				break;
+			}
+			hwdesc->cpos |= (JZ_LCD_CPOS_COEFFICIENT_1 <<
+					 JZ_LCD_CPOS_COEFFICIENT_OFFSET);
+			hwdesc->dessize =
+				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
+				FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK, height - 1) |
+				FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK, width - 1);
+		}
+
 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 			fourcc = newstate->fb->format->format;
 
@@ -693,6 +731,9 @@  static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
 	}
 
+	if (priv->soc_info->use_extended_hwdesc)
+		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
+
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
@@ -1015,6 +1056,7 @@  static int ingenic_drm_bind(struct device *dev, bool has_components)
 	long parent_rate;
 	unsigned int i, clone_mask = 0;
 	int ret, irq;
+	u32 osdc = 0;
 
 	soc_info = of_device_get_match_data(dev);
 	if (!soc_info) {
@@ -1272,7 +1314,10 @@  static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	/* Enable OSD if available */
 	if (soc_info->has_osd)
-		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
+		osdc |= JZ_LCD_OSDC_OSDEN;
+	if (soc_info->has_alpha)
+		osdc |= JZ_LCD_OSDC_ALPHAEN;
+	regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc);
 
 	mutex_init(&priv->clk_mutex);
 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
@@ -1468,10 +1513,24 @@  static const struct jz_soc_info jz4770_soc_info = {
 	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
 };
 
+static const struct jz_soc_info jz4780_soc_info = {
+	.needs_dev_clk = true,
+	.has_osd = true,
+	.has_alpha = true,
+	.use_extended_hwdesc = true,
+	.max_width = 4096,
+	.max_height = 2048,
+	.formats_f1 = jz4770_formats_f1,
+	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
+	.formats_f0 = jz4770_formats_f0,
+	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
+};
+
 static const struct of_device_id ingenic_drm_of_match[] = {
 	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
 	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
 	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
+	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
index 22654ac1dde1c..cb1d09b625881 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm.h
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
@@ -44,8 +44,11 @@ 
 #define JZ_REG_LCD_XYP1				0x124
 #define JZ_REG_LCD_SIZE0			0x128
 #define JZ_REG_LCD_SIZE1			0x12c
+#define JZ_REG_LCD_PCFG				0x2c0
 
 #define JZ_LCD_CFG_SLCD				BIT(31)
+#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
+#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
 #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
 #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
 #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
@@ -63,6 +66,7 @@ 
 #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
 #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
 #define JZ_LCD_CFG_18_BIT			BIT(7)
+#define JZ_LCD_CFG_24_BIT			BIT(6)
 #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
 
 #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
@@ -132,6 +136,7 @@ 
 #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
 #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
 #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
+#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
 
 #define JZ_LCD_SYNC_MASK			0x3ff
 
@@ -153,6 +158,7 @@ 
 #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
 
 #define JZ_LCD_OSDC_OSDEN			BIT(0)
+#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
 #define JZ_LCD_OSDC_F0EN			BIT(3)
 #define JZ_LCD_OSDC_F1EN			BIT(4)
 
@@ -176,6 +182,38 @@ 
 #define JZ_LCD_SIZE01_WIDTH_LSB			0
 #define JZ_LCD_SIZE01_HEIGHT_LSB		16
 
+#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
+#define JZ_LCD_DESSIZE_HEIGHT_MASK		GENMASK(23, 12)
+#define JZ_LCD_DESSIZE_WIDTH_MASK		GENMASK(11, 0)
+
+#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
+#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
+#define JZ_LCD_CPOS_BPP_30			(7 << 27)
+#define JZ_LCD_CPOS_RGB555			BIT(30)
+#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
+#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
+#define JZ_LCD_CPOS_COEFFICIENT_0		0
+#define JZ_LCD_CPOS_COEFFICIENT_1		1
+#define JZ_LCD_CPOS_COEFFICIENT_ALPHA1		2
+#define JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1	3
+
+#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
+#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
+#define JZ_LCD_RGBC_422				BIT(8)
+#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
+
+#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
+#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
+#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
+#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
+#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
+#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
+#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
+#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
+#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
+#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
+#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
+
 struct device;
 struct drm_plane;
 struct drm_plane_state;