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 <p...@crapouillou.net>:
>
> Hi Nikolaus,
>
> Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller
> <h...@goldelico.com> a écrit :
>> From: Paul Boddie <p...@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 <p...@boddie.org.uk>
>> Signed-off-by: Ezequiel Garcia <ezequ...@collabora.com>
>> Signed-off-by: H. Nikolaus Schaller <h...@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.
But then, why not do it right from the beginning?
>
>> +
>> + 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.
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
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.
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?
Why not having ingenic-ipu.c taking care of registering its driver as well?
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?
>
> Cheers,
> -Paul
BR and thanks,
Nikolaus
