Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Paul,
> Am 29.09.2021 um 16:30 schrieb Paul Cercueil :
>
> Hi,
>
> Le mar., sept. 28 2021 at 14:06:03 +0200, H. Nikolaus Schaller
> a écrit :
>> Hi Paul,
>>> Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller :
> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
> {
> int err;
> + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
> + err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
> + if (err)
> + return err;
> + }
I don't see why you need to register the ingenic-dw-hdmi driver here. Just
register it in the ingenic-dw-hdmi driver.
>>> Ok, I never though about this (as the code was not from me). We apparently
>>> just followed the IPU code pattern (learning by example).
>>> It indeed looks not necessary and would also avoid the
>>> ingenic_dw_hdmi_driver_ptr dependency.
>>> But: what is ingenic_ipu_driver_ptr then good for?
>
> It's done this way because ingenic-drm-drv.c and ingenic-ipu.c are both
> compiled within the same module ingenic-drm.
Ah, I see. Hadn't checked this.
> I'm not sure this is still required, maybe ingenic-ipu.c can be its own
> module now.
What I have seen is that it has its own compatible record. So there could be
load-on-demand by DTS.
>
>>> If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic:
>>> Fix drm_init error path if IPU was registered") completely.
>> A quick test shows that it *is* required. At least if I configure everything
>> as modules.
>> But like you I can't explain why.
>
> Well, a quick test here shows that it is not required, at least when
> configuring with everything built-in.
IMHO the hdmi driver (module) should be loaded on demand. Not everyone wants to
have it.
Well, that is the problem that needs to be solved...
BR and thanks,
Nikolaus
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi,
Le mar., sept. 28 2021 at 14:06:03 +0200, H. Nikolaus Schaller
a écrit :
Hi Paul,
Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller
:
@@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
{
int err;
+ if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
+ err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
+ if (err)
+ return err;
+ }
I don't see why you need to register the ingenic-dw-hdmi driver
here. Just register it in the ingenic-dw-hdmi driver.
Ok, I never though about this (as the code was not from me). We
apparently just followed the IPU code pattern (learning by example).
It indeed looks not necessary and would also avoid the
ingenic_dw_hdmi_driver_ptr dependency.
But: what is ingenic_ipu_driver_ptr then good for?
It's done this way because ingenic-drm-drv.c and ingenic-ipu.c are both
compiled within the same module ingenic-drm.
I'm not sure this is still required, maybe ingenic-ipu.c can be its own
module now.
If we can get rid of this as well, we can drop patch 1/10
("drm/ingenic: Fix drm_init error path if IPU was registered")
completely.
A quick test shows that it *is* required. At least if I configure
everything as modules.
But like you I can't explain why.
Well, a quick test here shows that it is not required, at least when
configuring with everything built-in.
-Paul
Well, just a very rough idea (may be wrong): the bridge chain is not
like an i2c bus and
clients are not automatically loaded/probed if linked in the device
tree. Therefore the
consumer (ingenic_drm_drv) must register the "clients" like IPU and
HDMI.
BR,
Nikolaus
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Paul,
> Am 28.09.2021 um 14:06 schrieb H. Nikolaus Schaller :
>
> Hi Paul,
>
>> Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller :
>>
@@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
{
int err;
+ if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
+ err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
+ if (err)
+ return err;
+ }
>>>
>>> I don't see why you need to register the ingenic-dw-hdmi driver here. Just
>>> register it in the ingenic-dw-hdmi driver.
>>
>> Ok, I never though about this (as the code was not from me). We apparently
>> just followed the IPU code pattern (learning by example).
>>
>> It indeed looks not necessary and would also avoid the
>> ingenic_dw_hdmi_driver_ptr dependency.
>>
>> But: what is ingenic_ipu_driver_ptr then good for?
>>
>> If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix
>> drm_init error path if IPU was registered") completely.
>
> A quick test shows that it *is* required. At least if I configure everything
> as modules.
> But like you I can't explain why.
>
> Well, just a very rough idea (may be wrong): the bridge chain is not like an
> i2c bus and
> clients are not automatically loaded/probed if linked in the device tree.
> Therefore the
> consumer (ingenic_drm_drv) must register the "clients" like IPU and HDMI.
Any suggestion how to proceed here for v5?
BR,
Nikolaus
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Paul,
> Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller :
>
>>> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void)
>>> {
>>> int err;
>>> + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
>>> + err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
>>> + if (err)
>>> + return err;
>>> + }
>>
>> I don't see why you need to register the ingenic-dw-hdmi driver here. Just
>> register it in the ingenic-dw-hdmi driver.
>
> Ok, I never though about this (as the code was not from me). We apparently
> just followed the IPU code pattern (learning by example).
>
> It indeed looks not necessary and would also avoid the
> ingenic_dw_hdmi_driver_ptr dependency.
>
> But: what is ingenic_ipu_driver_ptr then good for?
>
> If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix
> drm_init error path if IPU was registered") completely.
A quick test shows that it *is* required. At least if I configure everything as
modules.
But like you I can't explain why.
Well, just a very rough idea (may be wrong): the bridge chain is not like an
i2c bus and
clients are not automatically loaded/probed if linked in the device tree.
Therefore the
consumer (ingenic_drm_drv) must register the "clients" like IPU and HDMI.
BR,
Nikolaus
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi,
> Am 28.09.2021 um 11:35 schrieb Paul Cercueil :
>
> Hi Nikolaus / Paul,
>
> Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller
> a écrit :
>> From: Paul Boddie
>> 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
>> Signed-off-by: Ezequiel Garcia
>> Signed-off-by: H. Nikolaus Schaller
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +--
>> drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++
>> 2 files changed, 122 insertions(+), 5 deletions(-)
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index f73522bdacaa..e2df4b085905 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
>> #include
>> #include
>> #include
>> @@ -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 {
>> @@ -60,9 +66,11 @@ struct jz_soc_info {
>> bool needs_dev_clk;
>> bool has_osd;
>> 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;
>> +unsigned int max_reg;
>> };
>> struct ingenic_drm_private_state {
>> @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device
>> *dev, unsigned int reg)
>> }
>> }
>> -static const struct regmap_config ingenic_drm_regmap_config = {
>> +static struct regmap_config ingenic_drm_regmap_config = {
>> .reg_bits = 32,
>> .val_bits = 32,
>> .reg_stride = 4,
>> -.max_register = JZ_REG_LCD_SIZE1,
>> .writeable_reg = ingenic_drm_writeable_reg,
>> };
>> @@ -663,6 +670,37 @@ 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_XRGB:
>> +hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
>> +break;
>> +}
>> +hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
>> +(JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
>> + JZ_LCD_CPOS_COEFFICIENT_OFFSET);
>> +
>> +hwdesc->dessize =
>> +(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
>> +FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK <<
>> + JZ_LCD_DESSIZE_HEIGHT_OFFSET, height
>> - 1) |
>> +FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK <<
>> + JZ_LCD_DESSIZE_WIDTH_OFFSET, width -
>> 1);
>> +}
>> +
>> if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> fourcc = newstate->fb->format->format;
>> @@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct
>> drm_encoder *encoder,
>> | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> }
>> +/* set use of the 8-word descriptor and OSD foreground usage. */
>> +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)
>> @@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool
>> has_components)
>> struct drm_encoder *encoder;
>> struct ingenic_drm_bridge *ib;
>> struct drm_device *drm;
>> +struct regmap_config regmap_config;
>> void __iomem *base;
>> long parent_rate;
>> unsigned int i, clone_mask = 0;
>> @@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct de
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Nikolaus / Paul,
Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller
a écrit :
From: Paul Boddie
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
Signed-off-by: Ezequiel Garcia
Signed-off-by: H. Nikolaus Schaller
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85
+--
drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++
2 files changed, 122 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index f73522bdacaa..e2df4b085905 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
#include
#include
#include
@@ -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 {
@@ -60,9 +66,11 @@ struct jz_soc_info {
bool needs_dev_clk;
bool has_osd;
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;
+ unsigned int max_reg;
};
struct ingenic_drm_private_state {
@@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct
device *dev, unsigned int reg)
}
}
-static const struct regmap_config ingenic_drm_regmap_config = {
+static struct regmap_config ingenic_drm_regmap_config = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
- .max_register = JZ_REG_LCD_SIZE1,
.writeable_reg = ingenic_drm_writeable_reg,
};
@@ -663,6 +670,37 @@ 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_XRGB:
+ hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
+ break;
+ }
+ hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
+ (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
+JZ_LCD_CPOS_COEFFICIENT_OFFSET);
+
+ hwdesc->dessize =
+ (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
+ FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK <<
+ JZ_LCD_DESSIZE_HEIGHT_OFFSET, height
- 1) |
+ FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK <<
+ JZ_LCD_DESSIZE_WIDTH_OFFSET, width -
1);
+ }
+
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
fourcc = newstate->fb->format->format;
@@ -694,6 +732,10 @@ static void
ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
| JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
}
+ /* set use of the 8-word descriptor and OSD foreground usage. */
+ 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)
@@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev,
bool has_components)
struct drm_encoder *encoder;
struct ingenic_drm_bridge *ib;
struct drm_device *drm;
+ struct regmap_config regmap_config;
void __iomem *base;
long parent_rate;
unsigned int i, clone_mask = 0;
@@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device
*dev, bool has_components)
return PTR_ERR(base);
}
+ regmap_config = ingenic_drm_regmap_config;
+ regmap_config.max_register = soc_i
[PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
From: Paul Boddie
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
Signed-off-by: Ezequiel Garcia
Signed-off-by: H. Nikolaus Schaller
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +--
drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++
2 files changed, 122 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index f73522bdacaa..e2df4b085905 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
#include
#include
#include
@@ -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 {
@@ -60,9 +66,11 @@ struct jz_soc_info {
bool needs_dev_clk;
bool has_osd;
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;
+ unsigned int max_reg;
};
struct ingenic_drm_private_state {
@@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev,
unsigned int reg)
}
}
-static const struct regmap_config ingenic_drm_regmap_config = {
+static struct regmap_config ingenic_drm_regmap_config = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
- .max_register = JZ_REG_LCD_SIZE1,
.writeable_reg = ingenic_drm_writeable_reg,
};
@@ -663,6 +670,37 @@ 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_XRGB:
+ hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24;
+ break;
+ }
+ hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
+ (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 <<
+JZ_LCD_CPOS_COEFFICIENT_OFFSET);
+
+ hwdesc->dessize =
+ (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
+ FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK <<
+ JZ_LCD_DESSIZE_HEIGHT_OFFSET, height
- 1) |
+ FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK <<
+ JZ_LCD_DESSIZE_WIDTH_OFFSET, width -
1);
+ }
+
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
fourcc = newstate->fb->format->format;
@@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct
drm_encoder *encoder,
| JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
}
+ /* set use of the 8-word descriptor and OSD foreground usage. */
+ 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)
@@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool
has_components)
struct drm_encoder *encoder;
struct ingenic_drm_bridge *ib;
struct drm_device *drm;
+ struct regmap_config regmap_config;
void __iomem *base;
long parent_rate;
unsigned int i, clone_mask = 0;
@@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool
has_components)
return PTR_ERR(base);
}
+ regmap_config = ingenic_drm_regmap_config;
+ regmap_config.max_register = soc_info->max_reg;
priv->map = devm_regmap_init_mmio(dev, base,
-
