| Message ID | 2c7d0aa7d3ef480ebb996d37c27cbaa6f722728b.1633436959.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: JZ4780 and CI20 HDMI | expand |
Hi Nikolaus, Le mar., oct. 5 2021 at 14:29:14 +0200, 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 | 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); Knowing that OSD mode doesn't really work with this patch, I doubt you need to configure per-plane alpha blending. > + > + hwdesc->dessize = > + (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) | Same here. > + 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); Better pre-shift your *_MASK macros (and use GENMASK() in them) and remove the *_OFFSET macros. > + } > + > 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. */ I think you can remove this comment - this code doesn't actually set OSD mode, but it does enable the use of the extended hardware descriptor format, and I think it is already self-explanatory. > + 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); > + ®map_config); I remember saying to split this change into its own patch :) > 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); This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values. > > 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? */ From a quick look at the PMs, it doesn't seem so. > + .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; > + } As I said in your v4... You don't need to add this here. The ingenic-dw-hdmi.c should take care of registering its driver. As for the overall change... I am a bit annoyed that this only supports the F1 plane at the screen's resolution. Anything else (F1 plane at specific coordinates, F0 plane alone, or F0+F1) does not work. I think at least the F1 plane's position should be easy to do (just setting the cpos field in the hwdesc). It is OK to leave the rest for later (I'm not asking you to do all that). However it would be a good idea to add a check in ingenic_drm_crtc_atomic_check(), which would return -EINVAL if anything else than the working configuration is attempted. Cheers, -Paul > + > if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) { > err = platform_driver_register(ingenic_ipu_driver_ptr); > if (err) > - return err; > + goto err_hdmi_unreg; > } > > err = platform_driver_register(&ingenic_drm_driver); > @@ -1507,6 +1576,10 @@ static int ingenic_drm_init(void) > err_ipu_unreg: > if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) > platform_driver_unregister(ingenic_ipu_driver_ptr); > +err_hdmi_unreg: > + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) > + platform_driver_unregister(ingenic_dw_hdmi_driver_ptr); > + > return err; > } > module_init(ingenic_drm_init); > @@ -1515,6 +1588,8 @@ static void ingenic_drm_exit(void) > { > platform_driver_unregister(&ingenic_drm_driver); > > + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) > + platform_driver_unregister(ingenic_dw_hdmi_driver_ptr); > if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) > platform_driver_unregister(ingenic_ipu_driver_ptr); > } > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h > b/drivers/gpu/drm/ingenic/ingenic-drm.h > index 22654ac1dde1..13dbc5d25c3b 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,41 @@ > #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_OFFSET 12 > +#define JZ_LCD_DESSIZE_WIDTH_OFFSET 0 > +#define JZ_LCD_DESSIZE_HEIGHT_MASK 0xfff > +#define JZ_LCD_DESSIZE_WIDTH_MASK 0xfff > + > +/* TODO: 4,5 and 7 match the above BPP */ > +#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; > @@ -187,5 +228,6 @@ void ingenic_drm_plane_disable(struct device > *dev, struct drm_plane *plane); > bool ingenic_drm_map_noncoherent(const struct device *dev); > > extern struct platform_driver *ingenic_ipu_driver_ptr; > +extern struct platform_driver *ingenic_dw_hdmi_driver_ptr; > > #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */ > -- > 2.33.0 >
Hi Nikolaus, Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > sorry for the delay in getting back to this, but I was distracted by > more urgent topics. > >> Am 05.10.2021 um 22:22 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Nikolaus, >> >> Le mar., oct. 5 2021 at 14:29:14 +0200, 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 | 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 @@ > >>> 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); >> >> Knowing that OSD mode doesn't really work with this patch, I doubt >> you need to configure per-plane alpha blending. > > Well, we can not omit setting some CPOS_COEFFICIENT different from 0. > > This would mean to multiply all values with 0, i.e. gives a black > screen. > > So at least we have to apply JZ_LCD_CPOS_COEFFICIENT_1. > JZ_LCD_CPOS_PREMULTIPLY_LCD is not relevant in the non-alpha case. hwdesc->cpos = JZ_LCD_CPOS_COEFFICIENT_1 << JZ_LCD_CPOS_COEFFICIENT_OFFSET; That's enough to get an image. > But then, why not do it right from the beginning? Because there's no way to test alpha blending without getting the overlay plane to work first. >> >>> + >>> + hwdesc->dessize = >>> + (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) | >> >> Same here. >> >>> + 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); >> >> Better pre-shift your *_MASK macros (and use GENMASK() in them) and >> remove the *_OFFSET macros. > > Ok, I see. Nice. Makes code and definitions much cleaner. > Changed for v6. > >> >>> + } >>> + >>> 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. */ >> >> I think you can remove this comment - this code doesn't actually >> set OSD mode, but it does enable the use of the extended hardware >> descriptor format, and I think it is already self-explanatory. > > Agreed and removed. > >> >>> + 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); >>> + ®map_config); >> >> I remember saying to split this change into its own patch :) > > Yes, I remember as well, but it does not make sense to me. > > A first patch would introduce regmap_config. This needs > soc_info->max_reg > to be defined as a struct component. > > This requires all soc_info to be updated for all SoC (w/o > jz4780_soc_info > in this first patch because it has not been added yet) to a constant > (!) > JZ_REG_LCD_SIZE1. > > And the second patch would then add jz4780_soc_info and set its > max_reg to > a different value. Correct, that's how it should be. Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this. > IMHO, such a separate first patch has no benefit independent from > adding > jz4780 support, as far as I can see. > > If your fear issues with bisectability: > - code has been tested > - if this fails, bisect will still point to this patch, where it is > easy to locate It's not about bisectability. One functional change per patch, and patches should be as atomic as possible. > So I leave it in v6 unsplitted. > >> >>> 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); >> >> This change is unrelated to this patch, and I'm not even sure it's >> a valid change. The driver shouldn't rely on previous register >> values. > > I think I already commented that I think the driver should also not > reset > previous register values to zero. You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch. > If I counted correctly this register has 18 bits which seem to include > some interrupt masks (which could be initialized somewhere else) and > we > write a constant 0x1. > > Of course most other bits are clearly OSD related (alpha blending), > i.e. they can have any value (incl. 0) if OSD is disabled. But here we > enable it. I think there may be missing some setting for the other > bits. > > So are you sure, that we can unconditionally reset *all* bits > except JZ_LCD_OSDC_OSDEN for the jz4780? > > Well I have no experience with OSD being enabled at all. I.e. I have > no > test scenario. > > So we can leave it out in v6. > >> >>> 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? */ >> >> From a quick look at the PMs, it doesn't seem so. > > Fine. I'll remove the comment in v6. > >> >>> + .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; >>> + } >> >> As I said in your v4... You don't need to add this here. The >> ingenic-dw-hdmi.c should take care of registering its driver. > > Well, I can not identify any difference in code structure to the IPU > code. > > The Makefile (after our patch) looks like: > > obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o > ingenic-drm-y = ingenic-drm-drv.o > ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o > ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o > > which means that ingenic-dw-hdmi.o is also compiled into ingenic-drm, > like ingenic-drm-drv.o and ingenic-ipu.o - if CONFIGured. If not, > there > are these IS_ENABLED() tests to guard against compiler errors. > > Is there any technical reason to request a driver structure and > registration > different from IPU here? There is no reason to have ingenic-dw-hdmi built into the ingenic-drm module. It should be a separate module. > Why not having ingenic-ipu.c taking care of registering its driver as > well? IIRC ingenic-ipu.c was built into the ingenic-drm at the beginning because of circular dependencies between the IPU and main DRM driver. I think ingenic-ipu.c could be its own module now. That's something I will test soon. > As soon as this is clarified, I can post a v6. > >> >> As for the overall change... I am a bit annoyed that this only >> supports the F1 plane at the screen's resolution. Anything else (F1 >> plane at specific coordinates, F0 plane alone, or F0+F1) does not >> work. > > Yes, having two planes working would be interesting. > >> I think at least the F1 plane's position should be easy to do (just >> setting the cpos field in the hwdesc). >> >> It is OK to leave the rest for later (I'm not asking you to do all >> that). However it would be a good idea to add a check in >> ingenic_drm_crtc_atomic_check(), which would return -EINVAL if >> anything else than the working configuration is attempted. > > Hm. I am not familiar with how ingenic_drm_crtc_atomic_check() > would be notified about planes. Which configuration parameters > should be checked for? You know that the &ingenic_drm->f0 plane cannot be used (right now), so in ingenic_drm_plane_atomic_check() just: if (plane == &priv->f0 && crtc) return -EINVAL; Cheers, -Paul > >> >> Cheers, >> -Paul > > BR and thanks, > Nikolaus >
Hi Paul, > Am 07.11.2021 um 20:01 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >> sorry for the delay in getting back to this, but I was distracted by more urgent topics. >>> Am 05.10.2021 um 22:22 schrieb Paul Cercueil <paul@crapouillou.net>: >>> Hi Nikolaus, >>> Le mar., oct. 5 2021 at 14:29:14 +0200, 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 | 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 @@ >>>> 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); >>> Knowing that OSD mode doesn't really work with this patch, I doubt you need to configure per-plane alpha blending. >> Well, we can not omit setting some CPOS_COEFFICIENT different from 0. >> This would mean to multiply all values with 0, i.e. gives a black screen. >> So at least we have to apply JZ_LCD_CPOS_COEFFICIENT_1. >> JZ_LCD_CPOS_PREMULTIPLY_LCD is not relevant in the non-alpha case. > > hwdesc->cpos = JZ_LCD_CPOS_COEFFICIENT_1 << JZ_LCD_CPOS_COEFFICIENT_OFFSET; Exactly what I wrote and did test. > > That's enough to get an image. Fine that we can agree on that. > >> But then, why not do it right from the beginning? > > Because there's no way to test alpha blending without getting the overlay plane to work first. > >>> } >>> + 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); >>> + ®map_config); >>> I remember saying to split this change into its own patch :) >> Yes, I remember as well, but it does not make sense to me. >> A first patch would introduce regmap_config. This needs soc_info->max_reg >> to be defined as a struct component. >> This requires all soc_info to be updated for all SoC (w/o jz4780_soc_info >> in this first patch because it has not been added yet) to a constant (!) >> JZ_REG_LCD_SIZE1. >> And the second patch would then add jz4780_soc_info and set its max_reg to >> a different value. > > Correct, that's how it should be. Well, if you prefer separating things that are deeply related into two commits... > > Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this. That is an interesting idea. Although I don't see where https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171 does make use of the memory resource from DTS. It just reads two values from the ingenic_chip_info instead of one I do read from soc_info. If you see it I'd prefer to leave this patch to you (as it is not jz4780 related except that jz4780 needs it to be in place) and then I can simply make use of it for adding jz4780+hdmi. > >> IMHO, such a separate first patch has no benefit independent from adding >> jz4780 support, as far as I can see. >> If your fear issues with bisectability: >> - code has been tested >> - if this fails, bisect will still point to this patch, where it is easy to locate > > It's not about bisectability. One functional change per patch, and patches should be as atomic as possible. Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks... BTW: without adding jz4780_soc_info there is not even a functional change. Just the constant is made dependent on the .compatible entry. And since it is initialized to the same constant value in all cases, it is still a constant. A very very clever compiler could find out that regmap_config.max_register = soc_info->max_reg is a NOOP and produce the same code as before by avoiding the copy operation of regmap_config = ingenic_drm_regmap_config. > >> So I leave it in v6 unsplitted. >>>> 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); >>> This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values. >> I think I already commented that I think the driver should also not reset >> previous register values to zero. > > You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. > > Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch. Well it is in preparation of setting more bits that are only available for the jz4780. But it will be splitted into its own patch for other reasons - if we ever make osd working... > >> If I counted correctly this register has 18 bits which seem to include >> some interrupt masks (which could be initialized somewhere else) and we >> write a constant 0x1. >> Of course most other bits are clearly OSD related (alpha blending), >> i.e. they can have any value (incl. 0) if OSD is disabled. But here we >> enable it. I think there may be missing some setting for the other bits. >> So are you sure, that we can unconditionally reset *all* bits >> except JZ_LCD_OSDC_OSDEN for the jz4780? >> Well I have no experience with OSD being enabled at all. I.e. I have no >> test scenario. >> So we can leave it out in v6. So we agree as here well. >>>> >>>> + } >>> As I said in your v4... You don't need to add this here. The ingenic-dw-hdmi.c should take care of registering its driver. >> Well, I can not identify any difference in code structure to the IPU code. >> The Makefile (after our patch) looks like: >> obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o >> ingenic-drm-y = ingenic-drm-drv.o >> ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o >> ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o >> which means that ingenic-dw-hdmi.o is also compiled into ingenic-drm, >> like ingenic-drm-drv.o and ingenic-ipu.o - if CONFIGured. If not, there >> are these IS_ENABLED() tests to guard against compiler errors. >> Is there any technical reason to request a driver structure and registration >> different from IPU here? > > There is no reason to have ingenic-dw-hdmi built into the ingenic-drm module. It should be a separate module. > >> Why not having ingenic-ipu.c taking care of registering its driver as well? > > IIRC ingenic-ipu.c was built into the ingenic-drm at the beginning because of circular dependencies between the IPU and main DRM driver. I think ingenic-ipu.c could be its own module now. That's something I will test soon. Ok, that was the piece of information I was missing. I always thought that the way IPU is integrated is the best one and there is some special requirement. And it shows how we should do it. So I'll wait until I see your proposal for IPU. > >> As soon as this is clarified, I can post a v6. >> Hm. I am not familiar with how ingenic_drm_crtc_atomic_check() >> would be notified about planes. Which configuration parameters >> should be checked for? > > You know that the &ingenic_drm->f0 plane cannot be used (right now), so in ingenic_drm_plane_atomic_check() just: > > if (plane == &priv->f0 && crtc) > return -EINVAL; Ok, that is simple to add. Prepared for v6. So v6 is to be postponed by the patch for setting up regmap_config.max_register and the separation of the IPU driver so that it does not interfere. BR and thanks for all comments, Nikolaus
Hi Nikolaus, Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >> Am 07.11.2021 um 20:01 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Nikolaus, >> >> Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller >> <hns@goldelico.com> a écrit : >>> Hi Paul, >>> sorry for the delay in getting back to this, but I was distracted >>> by more urgent topics. >>>> Am 05.10.2021 um 22:22 schrieb Paul Cercueil >>>> <paul@crapouillou.net>: >>>> Hi Nikolaus, >>>> Le mar., oct. 5 2021 at 14:29:14 +0200, 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 | 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 @@ >>>>> 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); >>>> Knowing that OSD mode doesn't really work with this patch, I >>>> doubt you need to configure per-plane alpha blending. >>> Well, we can not omit setting some CPOS_COEFFICIENT different from >>> 0. >>> This would mean to multiply all values with 0, i.e. gives a black >>> screen. >>> So at least we have to apply JZ_LCD_CPOS_COEFFICIENT_1. >>> JZ_LCD_CPOS_PREMULTIPLY_LCD is not relevant in the non-alpha case. >> >> hwdesc->cpos = JZ_LCD_CPOS_COEFFICIENT_1 << >> JZ_LCD_CPOS_COEFFICIENT_OFFSET; > > Exactly what I wrote and did test. > >> >> That's enough to get an image. > > Fine that we can agree on that. > >> >>> But then, why not do it right from the beginning? >> >> Because there's no way to test alpha blending without getting the >> overlay plane to work first. >> >>>> } >>>> + 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); >>>> + ®map_config); >>>> I remember saying to split this change into its own patch :) >>> Yes, I remember as well, but it does not make sense to me. >>> A first patch would introduce regmap_config. This needs >>> soc_info->max_reg >>> to be defined as a struct component. >>> This requires all soc_info to be updated for all SoC (w/o >>> jz4780_soc_info >>> in this first patch because it has not been added yet) to a >>> constant (!) >>> JZ_REG_LCD_SIZE1. >>> And the second patch would then add jz4780_soc_info and set its >>> max_reg to >>> a different value. >> >> Correct, that's how it should be. > > Well, if you prefer separating things that are deeply related into > two commits... > >> >> Note that you can do even better, set the .max_register field >> according to the memory resource you get from DTS. Have a look at >> the pinctrl driver which does exactly this. > > That is an interesting idea. Although I don't see where > > https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171 > > does make use of the memory resource from DTS. It just reads two > values from the ingenic_chip_info instead of one I do read from > soc_info. It overrides the .max_register from a static regmap_config instance. You can do the same, calculating the .max_register from the memory resource you get from DT. I'm sure you guys can figure it out. > If you see it I'd prefer to leave this patch to you (as it is not > jz4780 related except that jz4780 needs it to be in place) and then I > can simply make use of it for adding jz4780+hdmi. > >> >>> IMHO, such a separate first patch has no benefit independent from >>> adding >>> jz4780 support, as far as I can see. >>> If your fear issues with bisectability: >>> - code has been tested >>> - if this fails, bisect will still point to this patch, where it >>> is easy to locate >> >> It's not about bisectability. One functional change per patch, and >> patches should be as atomic as possible. > > Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we > separate into "preparation for adding jz4780" and "really adding". > Yes, you can split atoms into quarks... And that's how it should be done. Lots of small atomic patches are much easier to review than a few big patches. > BTW: without adding jz4780_soc_info there is not even a functional > change. Just the constant is made dependent on the .compatible entry. > And since it is initialized to the same constant value in all cases, > it is still a constant. A very very clever compiler could find out > that regmap_config.max_register = soc_info->max_reg is a NOOP and > produce the same code as before by avoiding the copy operation of > regmap_config = ingenic_drm_regmap_config. > >> >>> So I leave it in v6 unsplitted. >>>>> 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); >>>> This change is unrelated to this patch, and I'm not even sure >>>> it's a valid change. The driver shouldn't rely on previous >>>> register values. >>> I think I already commented that I think the driver should also >>> not reset >>> previous register values to zero. >> >> You did comment this, yes, but I don't agree. The driver *should* >> reset the registers to zero. It should *not* have to rely on >> whatever was configured before. >> >> Even if I did agree, this is a functional change unrelated to >> JZ4780 support, so it would have to be splitted to its own patch. > > Well it is in preparation of setting more bits that are only > available for the jz4780. > > But it will be splitted into its own patch for other reasons - if we > ever make osd working... > >> >>> If I counted correctly this register has 18 bits which seem to >>> include >>> some interrupt masks (which could be initialized somewhere else) >>> and we >>> write a constant 0x1. >>> Of course most other bits are clearly OSD related (alpha blending), >>> i.e. they can have any value (incl. 0) if OSD is disabled. But >>> here we >>> enable it. I think there may be missing some setting for the other >>> bits. >>> So are you sure, that we can unconditionally reset *all* bits >>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>> Well I have no experience with OSD being enabled at all. I.e. I >>> have no >>> test scenario. >>> So we can leave it out in v6. > > So we agree as here well. > >>>>> >>>>> + } >>>> As I said in your v4... You don't need to add this here. The >>>> ingenic-dw-hdmi.c should take care of registering its driver. >>> Well, I can not identify any difference in code structure to the >>> IPU code. >>> The Makefile (after our patch) looks like: >>> obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o >>> ingenic-drm-y = ingenic-drm-drv.o >>> ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o >>> ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o >>> which means that ingenic-dw-hdmi.o is also compiled into >>> ingenic-drm, >>> like ingenic-drm-drv.o and ingenic-ipu.o - if CONFIGured. If not, >>> there >>> are these IS_ENABLED() tests to guard against compiler errors. >>> Is there any technical reason to request a driver structure and >>> registration >>> different from IPU here? >> >> There is no reason to have ingenic-dw-hdmi built into the >> ingenic-drm module. It should be a separate module. >> >>> Why not having ingenic-ipu.c taking care of registering its driver >>> as well? >> >> IIRC ingenic-ipu.c was built into the ingenic-drm at the beginning >> because of circular dependencies between the IPU and main DRM >> driver. I think ingenic-ipu.c could be its own module now. That's >> something I will test soon. > > Ok, that was the piece of information I was missing. I always thought > that the way IPU is integrated is the best one and there is some > special requirement. And it shows how we should do it. > > So I'll wait until I see your proposal for IPU. Don't need to wait for me - just create a standard platform_driver module for the HDMI code. Since it won't touch the ingenic-drm-drv.c file, if I later patch the IPU code to be its own module, it won't conflict. Cheers, -Paul >> >>> As soon as this is clarified, I can post a v6. >>> Hm. I am not familiar with how ingenic_drm_crtc_atomic_check() >>> would be notified about planes. Which configuration parameters >>> should be checked for? >> >> You know that the &ingenic_drm->f0 plane cannot be used (right >> now), so in ingenic_drm_plane_atomic_check() just: >> >> if (plane == &priv->f0 && crtc) >> return -EINVAL; > > Ok, that is simple to add. Prepared for v6. > > So v6 is to be postponed by the patch for setting up > regmap_config.max_register and the separation of the IPU driver so > that it does not interfere. > > BR and thanks for all comments, > Nikolaus >
Hi Paul, >> Am 08.11.2021 um 10:37 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks... > > And that's how it should be done. Lots of small atomic patches are much easier to review than a few big patches. I doubt that in this case especially as it has nothing to do with jz4780... But I have a proposal for a better solution at the end of this mail. >>> Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this. >> That is an interesting idea. Although I don't see where >> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171 >> does make use of the memory resource from DTS. It just reads two values from the ingenic_chip_info instead of one I do read from soc_info. > > It overrides the .max_register from a static regmap_config instance. To be precise: it overrides .max_register of a copy of a static regmap_config instance (which has .max_register = 0). > You can do the same, We already do the same... > calculating the .max_register from the memory resource you get from DT. I can't see any code in pinctrl-ingenic.c getting the memory resource that from DT. It calculates it from the ingenic_chip_info tables inside the driver code but not DT. > I'm sure you guys can figure it out. Ah, we have to figure out. You are not sure yourself how to do it? And it is *not* exactly like the pinctrl driver (already) does? Please give precise directions in reviews and not vague research homework. Our time is also valuable. Sorry if I review your reviews... Anyways I think you roughly intend (untested): struct resource *r; r = platform_get_resource(pdev, IORESOURCE_MEM, 0); regmap_config.max_register = r.end - r.start; But I wonder how that could work at all (despite adding code execution time) with: e.g. jz4770.dtsi: lcd: lcd-controller@13050000 { compatible = "ingenic,jz4770-lcd"; reg = <0x13050000 0x300>; or jz4725b.dtsi: lcd: lcd-controller@13050000 { compatible = "ingenic,jz4725b-lcd"; reg = <0x13050000 0x1000>; So max_register becomes 0x300 or 0x1000 but not #define JZ_REG_LCD_SIZE1 0x12c .max_reg = JZ_REG_LCD_SIZE1, And therefore wastes a lot of regmap memory. Do you want this? DTS should not be reduced (DTS should be kept as stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description). But here are good news: I have a simpler and less invasive proposal. We keep the devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals. This is then just a single-line change when introducing the jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review. Let's go this way to get it eventually finalized. Ok? BR and thanks, Nikolaus
Hi, Le lun., nov. 8 2021 at 11:52:20 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil >>> <paul@crapouillou.net>: >>> >>> Well, it was atomic: "add jz4780+hdmi functionality" or not. Now >>> we separate into "preparation for adding jz4780" and "really >>> adding". Yes, you can split atoms into quarks... >> >> And that's how it should be done. Lots of small atomic patches are >> much easier to review than a few big patches. > > I doubt that in this case especially as it has nothing to do with > jz4780... It has nothing to do with JZ4780 and that's exactly why it should be a separate patch. > But I have a proposal for a better solution at the end of this mail. > >>>> Note that you can do even better, set the .max_register field >>>> according to the memory resource you get from DTS. Have a look at >>>> the pinctrl driver which does exactly this. >>> That is an interesting idea. Although I don't see where >>> >>> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171 >>> does make use of the memory resource from DTS. It just reads two >>> values from the ingenic_chip_info instead of one I do read from >>> soc_info. >> >> It overrides the .max_register from a static regmap_config instance. > > To be precise: it overrides .max_register of a copy of a static > regmap_config instance (which has .max_register = 0). > >> You can do the same, > > We already do the same... > >> calculating the .max_register from the memory resource you get from >> DT. > > I can't see any code in pinctrl-ingenic.c getting the memory resource > that from DT. It calculates it from the ingenic_chip_info tables > inside the driver code but not DT. > >> I'm sure you guys can figure it out. > > Ah, we have to figure out. You are not sure yourself how to do it? > And it is *not* exactly like the pinctrl driver (already) does? > Please give precise directions in reviews and not vague research > homework. Our time is also valuable. Sorry if I review your reviews... > > Anyways I think you roughly intend (untested): > > struct resource *r; > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > regmap_config.max_register = r.end - r.start; Replace the "devm_platform_ioremap_resource" with "devm_platform_get_and_ioremap_resource" to get a pointer to the resource. Then the .max_register should be (r.end - r.start - 4) I think. And lose the aggressivity. It's not going to get you anywhere, especially since I'm the one who decides whether or not I should merge your patches. You want your code upstream, that's great, but it's your responsability to get it to shape so that it's eventually accepted. > > But I wonder how that could work at all (despite adding code > execution time) with: Code execution time? ... > e.g. jz4770.dtsi: > > lcd: lcd-controller@13050000 { > compatible = "ingenic,jz4770-lcd"; > reg = <0x13050000 0x300>; > > or jz4725b.dtsi: > > lcd: lcd-controller@13050000 { > compatible = "ingenic,jz4725b-lcd"; > reg = <0x13050000 0x1000>; > > So max_register becomes 0x300 or 0x1000 but not > > #define JZ_REG_LCD_SIZE1 0x12c > .max_reg = JZ_REG_LCD_SIZE1, > > And therefore wastes a lot of regmap memory. "regmap memory"? ... > Do you want this? DTS should not be reduced (DTS should be kept as > stable as possible), since the reg property describes address mapping > - not how many bytes are really used by registers or how big a cache > should be allocated (cache allocation size requirements are not > hardware description). The DTS should list the address and size of the register area. If your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130. > But here are good news: > > I have a simpler and less invasive proposal. We keep the > devm_regmap_init_mmio code as is and just increase its .max_register > from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. > This wastes a handful bytes for all non-jz4780 chips but less than > using the DTS memory region size. And is less code (no entry in > soc_info tables, no modifyable copy) and faster code execution than > all other proposals. > > This is then just a single-line change when introducing the jz4780. > And no "preparation for adding jz4780" patch is needed at all. No > patch to split out for separate review. > > Let's go this way to get it eventually finalized. Ok? No. Cheers, -Paul
Bnjour Paul, > Am 08.11.2021 um 13:20 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi, > > Le lun., nov. 8 2021 at 11:52:20 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >>>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil <paul@crapouillou.net>: >>>> Well, it was atomic: "add jz4780+hdmi functionality" or not. Now we separate into "preparation for adding jz4780" and "really adding". Yes, you can split atoms into quarks... >>> And that's how it should be done. Lots of small atomic patches are much easier to review than a few big patches. >> I doubt that in this case especially as it has nothing to do with jz4780... > > It has nothing to do with JZ4780 and that's exactly why it should be a separate patch. Question is why *I* should then make this patch and not someone else... I am not necessarily a volunteer to contribute to non-jz4780 code just because I want to upstream jz4780 code. > >> But I have a proposal for a better solution at the end of this mail. >>>>> Note that you can do even better, set the .max_register field according to the memory resource you get from DTS. Have a look at the pinctrl driver which does exactly this. >>>> That is an interesting idea. Although I don't see where >>>> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171 >>>> does make use of the memory resource from DTS. It just reads two values from the ingenic_chip_info instead of one I do read from soc_info. >>> It overrides the .max_register from a static regmap_config instance. >> To be precise: it overrides .max_register of a copy of a static regmap_config instance (which has .max_register = 0). >>> You can do the same, >> We already do the same... >>> calculating the .max_register from the memory resource you get from DT. >> I can't see any code in pinctrl-ingenic.c getting the memory resource that from DT. It calculates it from the ingenic_chip_info tables inside the driver code but not DT. >>> I'm sure you guys can figure it out. >> Ah, we have to figure out. You are not sure yourself how to do it? And it is *not* exactly like the pinctrl driver (already) does? Please give precise directions in reviews and not vague research homework. Our time is also valuable. Sorry if I review your reviews... >> Anyways I think you roughly intend (untested): >> struct resource *r; >> r = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> regmap_config.max_register = r.end - r.start; > > Replace the "devm_platform_ioremap_resource" with "devm_platform_get_and_ioremap_resource" to get a pointer to the resource. > > Then the .max_register should be (r.end - r.start - 4) I think. > > And lose the aggressivity. It's not going to get you anywhere, especially since I'm the one who decides whether or not I should merge your patches. You want your code upstream, that's great, but it's your responsability to get it to shape so that it's eventually accepted. Well on the other side of the fence it is maintainers responsibility to give clear and understandable rules and guidance about what will be accepted and to enable us to bring it into the shape he/she has in mind. But a fundamental problem is: "good shape" is very subjective. What would you recommend me to do, if I feel that my proposed code is in better shape than what the maintainer thinks or recommends? > >> But I wonder how that could work at all (despite adding code execution time) with: > > Code execution time? ... I reasoned about doing an additional platform_get_resource() call and doing a subtraction. This is additional execution time. Maybe not worth thinking about because it is in the probe function. And using devm_platform_get_and_ioremap_resource() is of course potentially better. > >> e.g. jz4770.dtsi: >> lcd: lcd-controller@13050000 { >> compatible = "ingenic,jz4770-lcd"; >> reg = <0x13050000 0x300>; >> or jz4725b.dtsi: >> lcd: lcd-controller@13050000 { >> compatible = "ingenic,jz4725b-lcd"; >> reg = <0x13050000 0x1000>; >> So max_register becomes 0x300 or 0x1000 but not >> #define JZ_REG_LCD_SIZE1 0x12c >> .max_reg = JZ_REG_LCD_SIZE1, >> And therefore wastes a lot of regmap memory. > > "regmap memory"? ... regmap allocates memory for its cache. Usually the total amount specified in the reg property. > >> Do you want this? DTS should not be reduced (DTS should be kept as stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description). > > The DTS should list the address and size of the register area. If your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130. If I look into some .dtsi it is sometimes that way but sometimes not. There seems to be no consistent rule. So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi and jz4725b.dtsi as well (as mentioned above: this is beyond the scope of my project)? > >> But here are good news: >> I have a simpler and less invasive proposal. We keep the devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals. >> This is then just a single-line change when introducing the jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review. >> Let's go this way to get it eventually finalized. Ok? > > No. Look friend, if you explain your "no" and what is wrong with my arguments, it helps to understand your decisions and learn something from them. A plain "no" does not help anyone. So to summarize: if you prefer something which I consider worse, it is ok for me... In the end you are right - you are the maintainer, not me. So you have to live with your proposals. Therefore, I have prepared new variants so you can choose which one is easier to maintain for you. Note that they are both preparing for full jz4780-lcdc/hdmi support but in very different ways: Variant 1 already adds some jz4780 stuff while Variant 2 just prepares for it. Variant 2 is not tested (except to compile). So it needs some Tested-by: from someone with access to hardware. IMHO it is more invasive. And don't forget: DTB could be in ROM or be provided by a separate bootloader... So we should not change it too often (I had such discussions some years ago with maintainers when I thought it is easier to change DTS instead of code). Variant 3 would be to not separate this. As proposed in [PATCH v5 2/7]. (Finally, a Variant 3b would be to combine the simple change from Variant 1 with Variant 3). So what is your choice? BR and thanks, Nikolaus #### VARIANT 0001 #### From c7afa925f6b53bb6cafa080b7dd582788c8eb2eb Mon Sep 17 00:00:00 2001 From: "H. Nikolaus Schaller" <hns@goldelico.com> Date: Mon, 8 Nov 2021 15:38:21 +0100 Subject: [PATCH] RFC: drm/ingenic: Add register definitions for JZ4780 lcdc JZ4780 has additional registers compared to the other SoC of the JZ47xx series. So we add the constants for registers and bits we make use of (there are even more which can be added later). And we increase the regmap max_register accordingly. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- drivers/gpu/drm/ingenic/ingenic-drm.h | 39 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index a5df1c8d34cde..1903e897d2299 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -122,7 +122,7 @@ static const struct regmap_config ingenic_drm_regmap_config = { .val_bits = 32, .reg_stride = 4, - .max_register = JZ_REG_LCD_SIZE1, + .max_register = JZ_REG_LCD_PCFG, .writeable_reg = ingenic_drm_writeable_reg, }; diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h index 22654ac1dde1c..e7430c4af41f6 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,39 @@ #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) + +/* TODO: 4,5 and 7 match the above BPP */ +#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;
Hi, Le lun., nov. 8 2021 at 16:29:11 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Bnjour Paul, > > >> Am 08.11.2021 um 13:20 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi, >> >> Le lun., nov. 8 2021 at 11:52:20 +0100, H. Nikolaus Schaller >> <hns@goldelico.com> a écrit : >>> Hi Paul, >>>>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil >>>>> <paul@crapouillou.net>: >>>>> Well, it was atomic: "add jz4780+hdmi functionality" or not. Now >>>>> we separate into "preparation for adding jz4780" and "really >>>>> adding". Yes, you can split atoms into quarks... >>>> And that's how it should be done. Lots of small atomic patches >>>> are much easier to review than a few big patches. >>> I doubt that in this case especially as it has nothing to do with >>> jz4780... >> >> It has nothing to do with JZ4780 and that's exactly why it should >> be a separate patch. > > Question is why *I* should then make this patch and not someone > else... > > I am not necessarily a volunteer to contribute to non-jz4780 code > just because I want to upstream jz4780 code. The JZ4780 patch lies on top of the other one, so they are still related. They just shouldn't be the same patch. >> >>> But I have a proposal for a better solution at the end of this >>> mail. >>>>>> Note that you can do even better, set the .max_register field >>>>>> according to the memory resource you get from DTS. Have a look >>>>>> at the pinctrl driver which does exactly this. >>>>> That is an interesting idea. Although I don't see where >>>>> >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinctrl-ingenic.c#L4171 >>>>> does make use of the memory resource from DTS. It just reads two >>>>> values from the ingenic_chip_info instead of one I do read from >>>>> soc_info. >>>> It overrides the .max_register from a static regmap_config >>>> instance. >>> To be precise: it overrides .max_register of a copy of a static >>> regmap_config instance (which has .max_register = 0). >>>> You can do the same, >>> We already do the same... >>>> calculating the .max_register from the memory resource you get >>>> from DT. >>> I can't see any code in pinctrl-ingenic.c getting the memory >>> resource that from DT. It calculates it from the ingenic_chip_info >>> tables inside the driver code but not DT. >>>> I'm sure you guys can figure it out. >>> Ah, we have to figure out. You are not sure yourself how to do it? >>> And it is *not* exactly like the pinctrl driver (already) does? >>> Please give precise directions in reviews and not vague research >>> homework. Our time is also valuable. Sorry if I review your >>> reviews... >>> Anyways I think you roughly intend (untested): >>> struct resource *r; >>> r = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> regmap_config.max_register = r.end - r.start; >> >> Replace the "devm_platform_ioremap_resource" with >> "devm_platform_get_and_ioremap_resource" to get a pointer to the >> resource. >> >> Then the .max_register should be (r.end - r.start - 4) I think. >> >> And lose the aggressivity. It's not going to get you anywhere, >> especially since I'm the one who decides whether or not I should >> merge your patches. You want your code upstream, that's great, but >> it's your responsability to get it to shape so that it's eventually >> accepted. > > Well on the other side of the fence it is maintainers responsibility > to give clear and understandable rules and guidance about what will > be accepted and to enable us to bring it into the shape he/she has in > mind. > > But a fundamental problem is: "good shape" is very subjective. What > would you recommend me to do, if I feel that my proposed code is in > better shape than what the maintainer thinks or recommends? > >> >>> But I wonder how that could work at all (despite adding code >>> execution time) with: >> >> Code execution time? ... > > I reasoned about doing an additional platform_get_resource() call and > doing a subtraction. This is additional execution time. Maybe not > worth thinking about because it is in the probe function. And using > devm_platform_get_and_ioremap_resource() is of course potentially > better. > >> >>> e.g. jz4770.dtsi: >>> lcd: lcd-controller@13050000 { >>> compatible = "ingenic,jz4770-lcd"; >>> reg = <0x13050000 0x300>; >>> or jz4725b.dtsi: >>> lcd: lcd-controller@13050000 { >>> compatible = "ingenic,jz4725b-lcd"; >>> reg = <0x13050000 0x1000>; >>> So max_register becomes 0x300 or 0x1000 but not >>> #define JZ_REG_LCD_SIZE1 0x12c >>> .max_reg = JZ_REG_LCD_SIZE1, >>> And therefore wastes a lot of regmap memory. >> >> "regmap memory"? ... > > regmap allocates memory for its cache. Usually the total amount > specified in the reg property. We are not using any register cache here. >> >>> Do you want this? DTS should not be reduced (DTS should be kept as >>> stable as possible), since the reg property describes address >>> mapping - not how many bytes are really used by registers or how >>> big a cache should be allocated (cache allocation size requirements >>> are not hardware description). >> >> The DTS should list the address and size of the register area. If >> your last register is at address 0x12c and there's nothing above, >> then the size in DTS should be 0x130. > > If I look into some .dtsi it is sometimes that way but sometimes not. > There seems to be no consistent rule. > > So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi and > jz4725b.dtsi as well (as mentioned above: this is beyond the scope of > my project)? You could update them if you wanted to, but there is no need to do it here. >> >>> But here are good news: >>> I have a simpler and less invasive proposal. We keep the >>> devm_regmap_init_mmio code as is and just increase its >>> .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when >>> introducing the jz4780. This wastes a handful bytes for all >>> non-jz4780 chips but less than using the DTS memory region size. >>> And is less code (no entry in soc_info tables, no modifyable copy) >>> and faster code execution than all other proposals. >>> This is then just a single-line change when introducing the >>> jz4780. And no "preparation for adding jz4780" patch is needed at >>> all. No patch to split out for separate review. >>> Let's go this way to get it eventually finalized. Ok? >> >> No. > > Look friend, if you explain your "no" and what is wrong with my > arguments, it helps to understand your decisions and learn something > from them. A plain "no" does not help anyone. I answered just "no" because I felt like I explained already what I wanted to see in the previous email. By using a huge number as the .max_register, we do *not* waste additional memory. Computing the value of the .max_register field does not add any overhead, either. The .max_register is only used for boundary checking. To make sure that you're not calling regmap_write() with an invalid register. That's all there is to it. > So to summarize: if you prefer something which I consider worse, it > is ok for me... In the end you are right - you are the maintainer, > not me. So you have to live with your proposals. > > Therefore, I have prepared new variants so you can choose which one > is easier to maintain for you. > > Note that they are both preparing for full jz4780-lcdc/hdmi support > but in very different ways: > > Variant 1 already adds some jz4780 stuff while Variant 2 just > prepares for it. > > Variant 2 is not tested (except to compile). So it needs some > Tested-by: from someone with access to hardware. IMHO it is more > invasive. > > And don't forget: DTB could be in ROM or be provided by a separate > bootloader... So we should not change it too often (I had such > discussions some years ago with maintainers when I thought it is > easier to change DTS instead of code). > > Variant 3 would be to not separate this. As proposed in [PATCH v5 > 2/7]. > (Finally, a Variant 3b would be to combine the simple change from > Variant 1 with Variant 3). > > So what is your choice? Variant 4: the variant #2 without the changes to the DTSI files. Cheers, -Paul > BR and thanks, > Nikolaus > > > #### VARIANT 0001 #### > > From c7afa925f6b53bb6cafa080b7dd582788c8eb2eb Mon Sep 17 00:00:00 2001 > From: "H. Nikolaus Schaller" <hns@goldelico.com> > Date: Mon, 8 Nov 2021 15:38:21 +0100 > Subject: [PATCH] RFC: drm/ingenic: Add register definitions for > JZ4780 lcdc > > JZ4780 has additional registers compared to the other > SoC of the JZ47xx series. So we add the constants for > registers and bits we make use of (there are even more > which can be added later). > > And we increase the regmap max_register accordingly. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- > drivers/gpu/drm/ingenic/ingenic-drm.h | 39 > +++++++++++++++++++++++ > 2 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index a5df1c8d34cde..1903e897d2299 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -122,7 +122,7 @@ static const struct regmap_config > ingenic_drm_regmap_config = { > .val_bits = 32, > .reg_stride = 4, > > - .max_register = JZ_REG_LCD_SIZE1, > + .max_register = JZ_REG_LCD_PCFG, > .writeable_reg = ingenic_drm_writeable_reg, > }; > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h > b/drivers/gpu/drm/ingenic/ingenic-drm.h > index 22654ac1dde1c..e7430c4af41f6 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,39 @@ > #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) > + > +/* TODO: 4,5 and 7 match the above BPP */ > +#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 > > > #### VARIANT 0002 #### > > From c4b5cfa2789493f02da91e385719bc97aefb6c6c Mon Sep 17 00:00:00 2001 > From: "H. Nikolaus Schaller" <hns@goldelico.com> > Date: Mon, 8 Nov 2021 14:40:58 +0100 > Subject: [PATCH] RFC: drm/ingenic: prepare ingenic drm for later > addition of > JZ4780 > > This changes the way the regmap is allocated to prepare for the > later addition of the JZ4780 which has more registers and bits > than the others. > > To make this work we have to change the device tree records of > all devices so that the reg property is limited to the physically > available registers. > > The magic value 0x130 in the device tree is JZ_REG_LCD_SIZE1 + 4. > > Note that it is not tested since I have no access to the hardware. > > Suggested-by: Paul Cercueil <paul@crapouillou.net> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > arch/mips/boot/dts/ingenic/jz4725b.dtsi | 2 +- > arch/mips/boot/dts/ingenic/jz4740.dtsi | 2 +- > arch/mips/boot/dts/ingenic/jz4770.dtsi | 2 +- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 9 ++++++--- > 4 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/arch/mips/boot/dts/ingenic/jz4725b.dtsi > b/arch/mips/boot/dts/ingenic/jz4725b.dtsi > index a1f0b71c92237..c017b087c0e52 100644 > --- a/arch/mips/boot/dts/ingenic/jz4725b.dtsi > +++ b/arch/mips/boot/dts/ingenic/jz4725b.dtsi > @@ -321,7 +321,7 @@ udc: usb@13040000 { > > lcd: lcd-controller@13050000 { > compatible = "ingenic,jz4725b-lcd"; > - reg = <0x13050000 0x1000>; > + reg = <0x13050000 0x130>; > > interrupt-parent = <&intc>; > interrupts = <31>; > diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi > b/arch/mips/boot/dts/ingenic/jz4740.dtsi > index c1afdfdaa8a38..ce3689e5015b5 100644 > --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi > +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi > @@ -323,7 +323,7 @@ udc: usb@13040000 { > > lcd: lcd-controller@13050000 { > compatible = "ingenic,jz4740-lcd"; > - reg = <0x13050000 0x1000>; > + reg = <0x13050000 0x130>; > > interrupt-parent = <&intc>; > interrupts = <30>; > diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi > b/arch/mips/boot/dts/ingenic/jz4770.dtsi > index 05c00b93088e9..0d1ee58d6c40b 100644 > --- a/arch/mips/boot/dts/ingenic/jz4770.dtsi > +++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi > @@ -399,7 +399,7 @@ gpu: gpu@13040000 { > > lcd: lcd-controller@13050000 { > compatible = "ingenic,jz4770-lcd"; > - reg = <0x13050000 0x300>; > + reg = <0x13050000 0x130>; > > interrupt-parent = <&intc>; > interrupts = <31>; > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index a5df1c8d34cde..3c8e3c5a447bb 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -122,7 +122,6 @@ static const struct regmap_config > ingenic_drm_regmap_config = { > .val_bits = 32, > .reg_stride = 4, > > - .max_register = JZ_REG_LCD_SIZE1, > .writeable_reg = ingenic_drm_writeable_reg, > }; > > @@ -858,6 +857,8 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > struct drm_encoder *encoder; > struct drm_device *drm; > void __iomem *base; > + struct resource *res; > + struct regmap_config regmap_config; > long parent_rate; > unsigned int i, clone_mask = 0; > dma_addr_t dma_hwdesc_phys_f0, dma_hwdesc_phys_f1; > @@ -904,14 +905,16 @@ static int ingenic_drm_bind(struct device *dev, > bool has_components) > drm->mode_config.funcs = &ingenic_drm_mode_config_funcs; > drm->mode_config.helper_private = &ingenic_drm_mode_config_helpers; > > - base = devm_platform_ioremap_resource(pdev, 0); > + base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(base)) { > dev_err(dev, "Failed to get memory resource\n"); > return PTR_ERR(base); > } > > + regmap_config = ingenic_drm_regmap_config; > + regmap_config.max_register = res->end - res->start - 4; > priv->map = devm_regmap_init_mmio(dev, base, > - &ingenic_drm_regmap_config); > + ®map_config); > if (IS_ERR(priv->map)) { > dev_err(dev, "Failed to create regmap\n"); > return PTR_ERR(priv->map); > -- > 2.33.0 > >
Hi Paul, > Am 08.11.2021 um 17:30 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi, > > Le lun., nov. 8 2021 at 16:29:11 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Bnjour Paul, >>> Am 08.11.2021 um 13:20 schrieb Paul Cercueil <paul@crapouillou.net>: >>> Hi, >>>> e.g. jz4770.dtsi: >>>> lcd: lcd-controller@13050000 { >>>> compatible = "ingenic,jz4770-lcd"; >>>> reg = <0x13050000 0x300>; >>>> or jz4725b.dtsi: >>>> lcd: lcd-controller@13050000 { >>>> compatible = "ingenic,jz4725b-lcd"; >>>> reg = <0x13050000 0x1000>; >>>> So max_register becomes 0x300 or 0x1000 but not >>>> #define JZ_REG_LCD_SIZE1 0x12c >>>> .max_reg = JZ_REG_LCD_SIZE1, >>>> And therefore wastes a lot of regmap memory. >>> "regmap memory"? ... >> regmap allocates memory for its cache. Usually the total amount specified in the reg property. > > We are not using any register cache here. > >>>> Do you want this? DTS should not be reduced (DTS should be kept as stable as possible), since the reg property describes address mapping - not how many bytes are really used by registers or how big a cache should be allocated (cache allocation size requirements are not hardware description). >>> The DTS should list the address and size of the register area. If your last register is at address 0x12c and there's nothing above, then the size in DTS should be 0x130. >> If I look into some .dtsi it is sometimes that way but sometimes not. There seems to be no consistent rule. >> So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi and jz4725b.dtsi as well (as mentioned above: this is beyond the scope of my project)? > > You could update them if you wanted to, but there is no need to do it here. Hm. Then we are changing the .max_register initialization to a much bigger value. > >>>> But here are good news: >>>> I have a simpler and less invasive proposal. We keep the devm_regmap_init_mmio code as is and just increase its .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when introducing the jz4780. This wastes a handful bytes for all non-jz4780 chips but less than using the DTS memory region size. And is less code (no entry in soc_info tables, no modifyable copy) and faster code execution than all other proposals. >>>> This is then just a single-line change when introducing the jz4780. And no "preparation for adding jz4780" patch is needed at all. No patch to split out for separate review. >>>> Let's go this way to get it eventually finalized. Ok? >>> No. >> Look friend, if you explain your "no" and what is wrong with my arguments, it helps to understand your decisions and learn something from them. A plain "no" does not help anyone. > > I answered just "no" because I felt like I explained already what I wanted to see in the previous email. > > By using a huge number as the .max_register, we do *not* waste additional memory. Computing the value of the .max_register field does not add any overhead, either. > > The .max_register is only used for boundary checking. To make sure that you're not calling regmap_write() with an invalid register. That's all there is to it. Ah, now I understand our disconnect. So far I have used regmaps mainly for i2c devices and there is caching to avoid redundant i2c traffic... So I just assumed wrongly that the regmap driver also allocates some buffer/cache here. Although it does not initialize .cache_type (default: REGCACHE_NONE). > >> So to summarize: if you prefer something which I consider worse, it is ok for me... In the end you are right - you are the maintainer, not me. So you have to live with your proposals. >> Therefore, I have prepared new variants so you can choose which one is easier to maintain for you. >> Note that they are both preparing for full jz4780-lcdc/hdmi support but in very different ways: >> Variant 1 already adds some jz4780 stuff while Variant 2 just prepares for it. >> Variant 2 is not tested (except to compile). So it needs some Tested-by: from someone with access to hardware. IMHO it is more invasive. >> And don't forget: DTB could be in ROM or be provided by a separate bootloader... So we should not change it too often (I had such discussions some years ago with maintainers when I thought it is easier to change DTS instead of code). >> Variant 3 would be to not separate this. As proposed in [PATCH v5 2/7]. >> (Finally, a Variant 3b would be to combine the simple change from Variant 1 with Variant 3). >> So what is your choice? > > Variant 4: the variant #2 without the changes to the DTSI files. Hm. If there is no cache and we can safely remove tight boundary checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) why do we still need the max_register calculation from DTSI specifically for jz4780 and at all? So what about: Variant 5: set .max_register = 0x1800, i.e. "big enough for everyone" (includes z4780 gamma and vee registers) + no DTSI changes (+ no jz4780 register constants like Variant 1) + no DTSI changes + no calculation from DTSI needed + single separate patch to prepare for jz4780 but not included in jz4780 patch BR and thanks, Nikolaus
Hi, Le lun., nov. 8 2021 at 18:22:58 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >> Am 08.11.2021 um 17:30 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi, >> >> Le lun., nov. 8 2021 at 16:29:11 +0100, H. Nikolaus Schaller >> <hns@goldelico.com> a écrit : >>> Bnjour Paul, >>>> Am 08.11.2021 um 13:20 schrieb Paul Cercueil >>>> <paul@crapouillou.net>: >>>> Hi, >>>>> e.g. jz4770.dtsi: >>>>> lcd: lcd-controller@13050000 { >>>>> compatible = "ingenic,jz4770-lcd"; >>>>> reg = <0x13050000 0x300>; >>>>> or jz4725b.dtsi: >>>>> lcd: lcd-controller@13050000 { >>>>> compatible = "ingenic,jz4725b-lcd"; >>>>> reg = <0x13050000 0x1000>; >>>>> So max_register becomes 0x300 or 0x1000 but not >>>>> #define JZ_REG_LCD_SIZE1 0x12c >>>>> .max_reg = JZ_REG_LCD_SIZE1, >>>>> And therefore wastes a lot of regmap memory. >>>> "regmap memory"? ... >>> regmap allocates memory for its cache. Usually the total amount >>> specified in the reg property. >> >> We are not using any register cache here. >> >>>>> Do you want this? DTS should not be reduced (DTS should be kept >>>>> as stable as possible), since the reg property describes address >>>>> mapping - not how many bytes are really used by registers or how >>>>> big a cache should be allocated (cache allocation size >>>>> requirements are not hardware description). >>>> The DTS should list the address and size of the register area. If >>>> your last register is at address 0x12c and there's nothing above, >>>> then the size in DTS should be 0x130. >>> If I look into some .dtsi it is sometimes that way but sometimes >>> not. There seems to be no consistent rule. >>> So does this mean you allow me to modify jz4740.dtsi, jz4770.dtsi >>> and jz4725b.dtsi as well (as mentioned above: this is beyond the >>> scope of my project)? >> >> You could update them if you wanted to, but there is no need to do >> it here. > > Hm. Then we are changing the .max_register initialization to a much > bigger value. > >> >>>>> But here are good news: >>>>> I have a simpler and less invasive proposal. We keep the >>>>> devm_regmap_init_mmio code as is and just increase its >>>>> .max_register from JZ_REG_LCD_SIZE1 to JZ_REG_LCD_PCFG when >>>>> introducing the jz4780. This wastes a handful bytes for all >>>>> non-jz4780 chips but less than using the DTS memory region size. >>>>> And is less code (no entry in soc_info tables, no modifyable >>>>> copy) and faster code execution than all other proposals. >>>>> This is then just a single-line change when introducing the >>>>> jz4780. And no "preparation for adding jz4780" patch is needed at >>>>> all. No patch to split out for separate review. >>>>> Let's go this way to get it eventually finalized. Ok? >>>> No. >>> Look friend, if you explain your "no" and what is wrong with my >>> arguments, it helps to understand your decisions and learn >>> something from them. A plain "no" does not help anyone. >> >> I answered just "no" because I felt like I explained already what I >> wanted to see in the previous email. >> >> By using a huge number as the .max_register, we do *not* waste >> additional memory. Computing the value of the .max_register field >> does not add any overhead, either. >> >> The .max_register is only used for boundary checking. To make sure >> that you're not calling regmap_write() with an invalid register. >> That's all there is to it. > > Ah, now I understand our disconnect. So far I have used regmaps > mainly for i2c devices and there is caching to avoid redundant i2c > traffic... > > So I just assumed wrongly that the regmap driver also allocates some > buffer/cache here. Although it does not initialize .cache_type > (default: REGCACHE_NONE). > >> >>> So to summarize: if you prefer something which I consider worse, >>> it is ok for me... In the end you are right - you are the >>> maintainer, not me. So you have to live with your proposals. >>> Therefore, I have prepared new variants so you can choose which >>> one is easier to maintain for you. >>> Note that they are both preparing for full jz4780-lcdc/hdmi >>> support but in very different ways: >>> Variant 1 already adds some jz4780 stuff while Variant 2 just >>> prepares for it. >>> Variant 2 is not tested (except to compile). So it needs some >>> Tested-by: from someone with access to hardware. IMHO it is more >>> invasive. >>> And don't forget: DTB could be in ROM or be provided by a separate >>> bootloader... So we should not change it too often (I had such >>> discussions some years ago with maintainers when I thought it is >>> easier to change DTS instead of code). >>> Variant 3 would be to not separate this. As proposed in [PATCH v5 >>> 2/7]. >>> (Finally, a Variant 3b would be to combine the simple change from >>> Variant 1 with Variant 3). >>> So what is your choice? >> >> Variant 4: the variant #2 without the changes to the DTSI files. > > Hm. If there is no cache and we can safely remove tight boundary > checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) > why do we still need the max_register calculation from DTSI > specifically for jz4780 and at all? It's better to have the .max_register actually set to the proper value. Then reading the registers from debugfs (/sys/kernel/debug/regmap/) will print the actual list of registers without bogus values. If .max_register is set too high, it will end up reading outside the registers area. On Ingenic SoCs such reads just return 0, but on some other SoCs it can lock up the system. So the best way forward is to have .max_register computed from the register area's size, and fix the DTSI with the proper sizes. Since your JZ4780 code needs to update .max_register anyway it's a good moment to add this patch, and the DTSI files can be fixed later (by me or whoever is up to the task). Fixing the DTS is not a problem in any way, btw. We just need to ensure that the drivers still work with old DTB files, which will be the case here. -Paul > So what about: > > Variant 5: set .max_register = 0x1800, i.e. "big enough for everyone" > (includes z4780 gamma and vee registers) + no DTSI changes (+ no > jz4780 register constants like Variant 1) > > + no DTSI changes > + no calculation from DTSI needed > + single separate patch to prepare for jz4780 but not included in > jz4780 patch > > BR and thanks, > Nikolaus > >
Hi Paul, > Am 08.11.2021 um 18:49 schrieb Paul Cercueil <paul@crapouillou.net>: > >>> Variant 4: the variant #2 without the changes to the DTSI files. >> Hm. If there is no cache and we can safely remove tight boundary checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing DTSI) why do we still need the max_register calculation from DTSI specifically for jz4780 and at all? > > It's better to have the .max_register actually set to the proper value. Then reading the registers from debugfs (/sys/kernel/debug/regmap/) will print the actual list of registers without bogus values. If .max_register is set too high, it will end up reading outside the registers area. Ok, that is a good reason to convince me. > On Ingenic SoCs such reads just return 0, but on some other SoCs it can lock up the system. Yes, I know some of these... > So the best way forward is to have .max_register computed from the register area's size, and fix the DTSI with the proper sizes. Since your JZ4780 code needs to update .max_register anyway it's a good moment to add this patch, and the DTSI files can be fixed later (by me or whoever is up to the task). Well, it would already be part of my Variant #2 (untested). So I could simply split it up further and you can test the pure dtsi changes and apply them later or modify if that makes problems. Saves you a little work. BTW: the jz4740 seems to have even less registers (last register seems to be LCDCMD1 @ 0x1305005C). > > Fixing the DTS is not a problem in any way, btw. We just need to ensure that the drivers still work with old DTB files, which will be the case here. Yes, that is right since the new values are smaller than the originals. Ok, then let's do it that way. BR and thanks, Nikolaus
Hi Nikolaus, Le lun., nov. 8 2021 at 19:33:48 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >> Am 08.11.2021 um 18:49 schrieb Paul Cercueil <paul@crapouillou.net>: >> >>>> Variant 4: the variant #2 without the changes to the DTSI files. >>> Hm. If there is no cache and we can safely remove tight boundary >>> checking (by JZ_REG_LCD_SIZE1) for jz4725/40/70 (by not fixing >>> DTSI) why do we still need the max_register calculation from DTSI >>> specifically for jz4780 and at all? >> >> It's better to have the .max_register actually set to the proper >> value. Then reading the registers from debugfs >> (/sys/kernel/debug/regmap/) will print the actual list of registers >> without bogus values. If .max_register is set too high, it will end >> up reading outside the registers area. > > Ok, that is a good reason to convince me. > >> On Ingenic SoCs such reads just return 0, but on some other SoCs it >> can lock up the system. > > Yes, I know some of these... > >> So the best way forward is to have .max_register computed from the >> register area's size, and fix the DTSI with the proper sizes. Since >> your JZ4780 code needs to update .max_register anyway it's a good >> moment to add this patch, and the DTSI files can be fixed later (by >> me or whoever is up to the task). > > Well, it would already be part of my Variant #2 (untested). So I > could simply split it up further and you can test the pure dtsi > changes and apply them later or modify if that makes problems. Saves > you a little work. BTW: the jz4740 seems to have even less registers > (last register seems to be LCDCMD1 @ 0x1305005C). Sure, if you want. Send the DTSI patch(es) separate from this patchset then. >> >> Fixing the DTS is not a problem in any way, btw. We just need to >> ensure that the drivers still work with old DTB files, which will be >> the case here. > > Yes, that is right since the new values are smaller than the > originals. > > Ok, then let's do it that way. Great. Waiting for your v6 then. Cheers, -Paul
Hi Paul, sorry to go back here... DDC power control is working now (and I now understand that a typo in my work-in-progress ci20.dts had switched the DDC SDA to active "1" without powering DDC and that may be beyond what the monitor wanted to see and therefore DDC edid isn't reliable any more... Other HDMI devices are more probably robust). I have also tested HPD and could make events passed to user-space by setting poll_mode. There is one subtle thing in ingenic-drm-drv I do not yet understand: > Am 08.11.2021 um 10:37 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >>>>>> >>>>>> @@ -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); >>>>> This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values. >>>> I think I already commented that I think the driver should also not reset >>>> previous register values to zero. >>> You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. >>> Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch. >> Well it is in preparation of setting more bits that are only available for the jz4780. >> But it will be splitted into its own patch for other reasons - if we ever make osd working... >>>> If I counted correctly this register has 18 bits which seem to include >>>> some interrupt masks (which could be initialized somewhere else) and we >>>> write a constant 0x1. >>>> Of course most other bits are clearly OSD related (alpha blending), >>>> i.e. they can have any value (incl. 0) if OSD is disabled. But here we >>>> enable it. I think there may be missing some setting for the other bits. >>>> So are you sure, that we can unconditionally reset *all* bits >>>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>>> Well I have no experience with OSD being enabled at all. I.e. I have no >>>> test scenario. It turns out that the line ret = clk_prepare_enable(priv->lcd_clk); initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 before). and writing regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); overwrites it with 0x00000001. This makes the HDMI monitor go/stay black until I write manually 0x5 to JZ_REG_LCD_OSDC. This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as well. Hence overwriting with JZ_LCD_OSDC_OSDEN breaks it. I didn't notice before because my test setup had some additional private patches + reverts and it did not properly revert to regmap_write. Now the questions: a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why is it needed? Is this a not well documented difference between jz4740/60/70/80? b) how can clk_prepare_enable() write 0x00010005 to JZ_REG_LCD_OSDC? Bug or feature? Something in cgu driver going wrong? c) what do your SoC or panels do if you write 0x5 to JZ_REG_LCD_OSDC? d) 0x00010005 also has bit 16 set which is undocumented... But this is a don't care according to jz4780 PM. BR and thanks, Nikolaus
Hi Paul, any insights on the JZ_REG_LCD_OSDC issue below? There is a second, unrelated issue with the introduction of "drm/bridge: display-connector: implement bus fmts callbacks" which breaks bus format negotiations. These are the two last unsolved issues to submit a fully working driver. > Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > >> Am 08.11.2021 um 10:37 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Nikolaus, >> >> Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>> Hi Paul, >>>>>>> >>>>>>> @@ -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); >>>>>> This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values. >>>>> I think I already commented that I think the driver should also not reset >>>>> previous register values to zero. >>>> You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. >>>> Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch. >>> Well it is in preparation of setting more bits that are only available for the jz4780. >>> But it will be splitted into its own patch for other reasons - if we ever make osd working... >>>>> If I counted correctly this register has 18 bits which seem to include >>>>> some interrupt masks (which could be initialized somewhere else) and we >>>>> write a constant 0x1. >>>>> Of course most other bits are clearly OSD related (alpha blending), >>>>> i.e. they can have any value (incl. 0) if OSD is disabled. But here we >>>>> enable it. I think there may be missing some setting for the other bits. >>>>> So are you sure, that we can unconditionally reset *all* bits >>>>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>>>> Well I have no experience with OSD being enabled at all. I.e. I have no >>>>> test scenario. > > It turns out that the line > > ret = clk_prepare_enable(priv->lcd_clk); > > initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 before). more detailled test shows that it is the underlying clk_enable(priv->lcd_clk) (i.e. not the prepare). > > and writing > > regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); > > overwrites it with 0x00000001. > > This makes the HDMI monitor go/stay black until I write manually 0x5 to > JZ_REG_LCD_OSDC. > > This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as well. > Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it. > > Now the questions: > a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why is it needed? > Is this a not well documented difference between jz4740/60/70/80? > b) how can clk_prepare_enable() write 0x00010005 to JZ_REG_LCD_OSDC? Bug or feature? > Something in cgu driver going wrong? I now suspect that it is an undocumented SoC feature. > c) what do your SoC or panels do if you write 0x5 to JZ_REG_LCD_OSDC? > d) 0x00010005 also has bit 16 set which is undocumented... But this is a don't care > according to jz4780 PM. BR and thanks, Nikolaus
Hi Nikolaus, Le mar., janv. 18 2022 at 15:50:01 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > any insights on the JZ_REG_LCD_OSDC issue below? Sorry, I missed your previous email. I blame the holidays ;) > There is a second, unrelated issue with the introduction of > > "drm/bridge: display-connector: implement bus fmts callbacks" > > which breaks bus format negotiations. > > These are the two last unsolved issues to submit a fully working > driver. > >> Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller >> <hns@goldelico.com>: >> >>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil >>> <paul@crapouillou.net>: >>> >>> Hi Nikolaus, >>> >>> Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller >>> <hns@goldelico.com> a écrit : >>>> Hi Paul, >>>>>>>> >>>>>>>> @@ -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); >>>>>>> This change is unrelated to this patch, and I'm not even sure >>>>>>> it's a valid change. The driver shouldn't rely on previous >>>>>>> register values. >>>>>> I think I already commented that I think the driver should also >>>>>> not reset >>>>>> previous register values to zero. >>>>> You did comment this, yes, but I don't agree. The driver >>>>> *should* reset the registers to zero. It should *not* have to >>>>> rely on whatever was configured before. >>>>> Even if I did agree, this is a functional change unrelated to >>>>> JZ4780 support, so it would have to be splitted to its own patch. >>>> Well it is in preparation of setting more bits that are only >>>> available for the jz4780. >>>> But it will be splitted into its own patch for other reasons - if >>>> we ever make osd working... >>>>>> If I counted correctly this register has 18 bits which seem to >>>>>> include >>>>>> some interrupt masks (which could be initialized somewhere >>>>>> else) and we >>>>>> write a constant 0x1. >>>>>> Of course most other bits are clearly OSD related (alpha >>>>>> blending), >>>>>> i.e. they can have any value (incl. 0) if OSD is disabled. But >>>>>> here we >>>>>> enable it. I think there may be missing some setting for the >>>>>> other bits. >>>>>> So are you sure, that we can unconditionally reset *all* bits >>>>>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>>>>> Well I have no experience with OSD being enabled at all. I.e. I >>>>>> have no >>>>>> test scenario. >> >> It turns out that the line >> >> ret = clk_prepare_enable(priv->lcd_clk); >> >> initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 >> before). > > more detailled test shows that it is the underlying > > clk_enable(priv->lcd_clk) > > (i.e. not the prepare). >> >> and writing >> >> regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); >> >> overwrites it with 0x00000001. >> >> This makes the HDMI monitor go/stay black until I write manually >> 0x5 to >> JZ_REG_LCD_OSDC. >> >> This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as >> well. >> Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it. >> >> Now the questions: >> a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why >> is it needed? >> Is this a not well documented difference between jz4740/60/70/80? >> b) how can clk_prepare_enable() write 0x00010005 to >> JZ_REG_LCD_OSDC? Bug or feature? >> Something in cgu driver going wrong? > > I now suspect that it is an undocumented SoC feature. Not at all. If the clock is disabled, the LCD controller is disabled, so all the registers read zero, this makes sense. You can only read the registers when the clock is enabled. On some SoCs, reading disabled registers can even cause a complete lockup. Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required, but that wouldn't be surprising. Anyway, feel free to send a patch to fix it (with a Fixes: tag). Ideally something like this: u32 osdc = 0; ... if (soc_info->has_osd) osdc |= JZ_LCD_OSDC_OSDEN; if (soc_info->has_alpha) osdc |= JZ_LCD_OSDC_ALPHAEN; regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc); This also ensures that the OSDC register is properly initialized in the !has_osd case. The driver shouldn't rely on pre-boot values in the registers. Cheers, -Paul > >> c) what do your SoC or panels do if you write 0x5 to >> JZ_REG_LCD_OSDC? >> d) 0x00010005 also has bit 16 set which is undocumented... But this >> is a don't care >> according to jz4780 PM. > > BR and thanks, > Nikolaus >
Hi Paul, > Am 18.01.2022 um 17:58 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le mar., janv. 18 2022 at 15:50:01 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >> any insights on the JZ_REG_LCD_OSDC issue below? > > Sorry, I missed your previous email. I blame the holidays ;) No problem... We all had deserved them. > >> There is a second, unrelated issue with the introduction of >> "drm/bridge: display-connector: implement bus fmts callbacks" >> which breaks bus format negotiations. >> These are the two last unsolved issues to submit a fully working driver. >>> Am 22.12.2021 um 15:03 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >>>> Am 08.11.2021 um 10:37 schrieb Paul Cercueil <paul@crapouillou.net>: >>>> Hi Nikolaus, >>>> Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>>> Hi Paul, >>>>>>>>> @@ -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); >>>>>>>> This change is unrelated to this patch, and I'm not even sure it's a valid change. The driver shouldn't rely on previous register values. >>>>>>> I think I already commented that I think the driver should also not reset >>>>>>> previous register values to zero. >>>>>> You did comment this, yes, but I don't agree. The driver *should* reset the registers to zero. It should *not* have to rely on whatever was configured before. >>>>>> Even if I did agree, this is a functional change unrelated to JZ4780 support, so it would have to be splitted to its own patch. >>>>> Well it is in preparation of setting more bits that are only available for the jz4780. >>>>> But it will be splitted into its own patch for other reasons - if we ever make osd working... >>>>>>> If I counted correctly this register has 18 bits which seem to include >>>>>>> some interrupt masks (which could be initialized somewhere else) and we >>>>>>> write a constant 0x1. >>>>>>> Of course most other bits are clearly OSD related (alpha blending), >>>>>>> i.e. they can have any value (incl. 0) if OSD is disabled. But here we >>>>>>> enable it. I think there may be missing some setting for the other bits. >>>>>>> So are you sure, that we can unconditionally reset *all* bits >>>>>>> except JZ_LCD_OSDC_OSDEN for the jz4780? >>>>>>> Well I have no experience with OSD being enabled at all. I.e. I have no >>>>>>> test scenario. >>> It turns out that the line >>> ret = clk_prepare_enable(priv->lcd_clk); >>> initializes JZ_REG_LCD_OSDC to 0x00010005 (i.e. printk tells 0x0 before). >> more detailled test shows that it is the underlying >> clk_enable(priv->lcd_clk) >> (i.e. not the prepare). >>> and writing >>> regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); >>> overwrites it with 0x00000001. >>> This makes the HDMI monitor go/stay black until I write manually 0x5 to >>> JZ_REG_LCD_OSDC. >>> This means that JZ_LCD_OSDC_ALPHAEN must be enabled on jz4780 as well. >>> Hence overwriting just with JZ_LCD_OSDC_OSDEN breaks it. >>> Now the questions: >>> a) 0x5 is understandable that it sets JZ_LCD_OSDC_ALPHAEN - but why is it needed? >>> Is this a not well documented difference between jz4740/60/70/80? >>> b) how can clk_prepare_enable() write 0x00010005 to JZ_REG_LCD_OSDC? Bug or feature? >>> Something in cgu driver going wrong? >> I now suspect that it is an undocumented SoC feature. > > Not at all. If the clock is disabled, the LCD controller is disabled, so all the registers read zero, this makes sense. You can only read the registers when the clock is enabled. On some SoCs, reading disabled registers can even cause a complete lockup. This is the right answer to the wrong question :) The question is why the register becomes 0x10005 as soon as the clock is enabled. Without writing to it. And contrary to the documented reset state. Or are you aware that u-boot initialized the lcdc and clocks get disabled when/during starting the kernel (I am using the good old v2013.10)? That could be an explanation that we read 0 before the clock is enabled and 0x10005 after. > Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working fine last time I tried, and now I indeed get a black screen unless this bit is set. The PM doesn't make it obvious that the bit is required, exactly. > but that wouldn't be surprising. > > Anyway, feel free to send a patch to fix it (with a Fixes: tag). Ideally something like this: > > u32 osdc = 0; > ... > if (soc_info->has_osd) > osdc |= JZ_LCD_OSDC_OSDEN; > if (soc_info->has_alpha) > osdc |= JZ_LCD_OSDC_ALPHAEN; > regmap_write(priv->map, JZ_REG_LCD_OSDC, osdc); Looks good to me. I'll give it a try. > > This also ensures that the OSDC register is properly initialized in the !has_osd case. The driver shouldn't rely on pre-boot values in the registers. Ok. Not all SoC have osd. BR and thanks, Nikolaus
Hi Paul, > Am 18.01.2022 um 23:59 schrieb Paul Boddie <paul@boddie.org.uk>: > > On Tuesday, 18 January 2022 17:58:58 CET Paul Cercueil wrote: >> >> Not at all. If the clock is disabled, the LCD controller is disabled, >> so all the registers read zero, this makes sense. You can only read the >> registers when the clock is enabled. On some SoCs, reading disabled >> registers can even cause a complete lockup. > > My concern was that something might be accessing the registers before the > clock had been enabled. It seems unlikely, given that the clock is enabled in > the bind function, and I would have thought that nothing would invoke the > different driver operations ("funcs") until bind has been called, nor should > anything called from within bind itself be accessing registers. > >> Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working >> fine last time I tried, and now I indeed get a black screen unless this >> bit is set. The PM doesn't make it obvious that the bit is required, >> but that wouldn't be surprising. > > It isn't actually needed. If the DMA descriptors are set up appropriately, the > OSD alpha bit seems to be set as a consequence. In my non-Linux testing > environment I don't even set any OSD registers explicitly, but the OSD alpha > and enable flags become set when the display is active. Is it set by DMA descriptors or by explicit code? We did have an explicit setting of JZ_LCD_OSDC_ALPHAEN https://www.spinics.net/lists/devicetree/msg438447.html but that was postponed for further discussion. And now if we add it (from basic functionality) back, it is fine again. BR, Nikolaus
On Wednesday, 19 January 2022 07:40:22 CET H. Nikolaus Schaller wrote: > Hi Paul, > > > Am 18.01.2022 um 23:59 schrieb Paul Boddie <paul@boddie.org.uk>: > > > > On Tuesday, 18 January 2022 17:58:58 CET Paul Cercueil wrote: > >> > >> Why is this JZ_LCD_OSDC_ALPHAEN bit needed now? I remember it working > >> fine last time I tried, and now I indeed get a black screen unless this > >> bit is set. The PM doesn't make it obvious that the bit is required, > >> but that wouldn't be surprising. > > > > It isn't actually needed. If the DMA descriptors are set up appropriately, > > the OSD alpha bit seems to be set as a consequence. In my non-Linux > > testing environment I don't even set any OSD registers explicitly, but > > the OSD alpha and enable flags become set when the display is active. > > Is it set by DMA descriptors or by explicit code? The descriptors will cause it to be set when the peripheral is enabled, as far as I can tell. > We did have an explicit setting of JZ_LCD_OSDC_ALPHAEN > > https://www.spinics.net/lists/devicetree/msg438447.html > > but that was postponed for further discussion. And now if we > add it (from basic functionality) back, it is fine again. It may be set in various versions of the Linux driver, but my observation was that in a non-Linux environment where nothing else is setting anything in the register concerned, initialising the descriptors seems to enable OSD and the OSD alpha enable bit. Yesterday, I did consider what might be done to avoid the alpha bit being set, but I didn't immediately see anything in the descriptor fields that would offer such an alternative. The bit in question seems to be a global alpha enable setting, and so choosing per-pixel alpha would probably also result in it being set, although I didn't fire up the CI20 to check. Paul
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); + ®map_config); 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); 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; + } + if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) { err = platform_driver_register(ingenic_ipu_driver_ptr); if (err) - return err; + goto err_hdmi_unreg; } err = platform_driver_register(&ingenic_drm_driver); @@ -1507,6 +1576,10 @@ static int ingenic_drm_init(void) err_ipu_unreg: if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) platform_driver_unregister(ingenic_ipu_driver_ptr); +err_hdmi_unreg: + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) + platform_driver_unregister(ingenic_dw_hdmi_driver_ptr); + return err; } module_init(ingenic_drm_init); @@ -1515,6 +1588,8 @@ static void ingenic_drm_exit(void) { platform_driver_unregister(&ingenic_drm_driver); + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) + platform_driver_unregister(ingenic_dw_hdmi_driver_ptr); if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) platform_driver_unregister(ingenic_ipu_driver_ptr); } diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h index 22654ac1dde1..13dbc5d25c3b 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,41 @@ #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_OFFSET 12 +#define JZ_LCD_DESSIZE_WIDTH_OFFSET 0 +#define JZ_LCD_DESSIZE_HEIGHT_MASK 0xfff +#define JZ_LCD_DESSIZE_WIDTH_MASK 0xfff + +/* TODO: 4,5 and 7 match the above BPP */ +#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; @@ -187,5 +228,6 @@ void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane); bool ingenic_drm_map_noncoherent(const struct device *dev); extern struct platform_driver *ingenic_ipu_driver_ptr; +extern struct platform_driver *ingenic_dw_hdmi_driver_ptr; #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */