Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output

From: H. Nikolaus Schaller
Date: Tue Sep 28 2021 - 06:22:22 EST

Hi,

> Am 28.09.2021 um 11:35 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
>
> Hi Nikolaus / Paul,
>
> Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> a écrit :
>> From: Paul Boddie <paul@xxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>> ---
>> 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 <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 {
>> @@ -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_XRGB8888:
>> + 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,
>> - &ingenic_drm_regmap_config);
>> + &regmap_config);
>
> Could you split the code that makes .max_reg configurable per-SoC into its own patch?

Yes.

>
>> if (IS_ERR(priv->map)) {
>> dev_err(dev, "Failed to create regmap\n");
>> return PTR_ERR(priv->map);
>> @@ -1274,7 +1319,7 @@ 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);
>> + regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>
> Why?

If I remember we should not assume that all others bits in JZ_REG_LCD_OSDC
can be safely overwritten by 0, although their reset state is 0 as well.

These are several alpha-blending bits and interrupt masks in the same register.
Apparently only in jz4780.

>
>> mutex_init(&priv->clk_mutex);
>> priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1444,6 +1489,7 @@ static const struct jz_soc_info jz4740_soc_info = {
>> .formats_f1 = jz4740_formats,
>> .num_formats_f1 = ARRAY_SIZE(jz4740_formats),
>> /* JZ4740 has only one plane */
>> + .max_reg = JZ_REG_LCD_SIZE1,
>> };
>> static const struct jz_soc_info jz4725b_soc_info = {
>> @@ -1456,6 +1502,7 @@ static const struct jz_soc_info jz4725b_soc_info = {
>> .num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1),
>> .formats_f0 = jz4725b_formats_f0,
>> .num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),
>> + .max_reg = JZ_REG_LCD_SIZE1,
>> };
>> static const struct jz_soc_info jz4770_soc_info = {
>> @@ -1468,12 +1515,28 @@ static const struct jz_soc_info jz4770_soc_info = {
>> .num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>> .formats_f0 = jz4770_formats_f0,
>> .num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
>> + .max_reg = JZ_REG_LCD_SIZE1,
>> +};
>> +
>> +static const struct jz_soc_info jz4780_soc_info = {
>> + .needs_dev_clk = true,
>> + .has_osd = true,
>> + .use_extended_hwdesc = true,
>> + .max_width = 4096,
>> + .max_height = 2048,
>> + /* REVISIT: do we support formats different from jz4770? */
>> + .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),
>> + .max_reg = JZ_REG_LCD_PCFG,
>> };
>> 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);
>> @@ -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.

>
> Cheers,
> -Paul
>

BR,
Nikolaus