Re: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output
From: Paul Cercueil
Date: Sun Nov 07 2021 - 14:02:26 EST
Hi Nikolaus,
Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller
<hns@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>:
Hi Nikolaus,
Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller
<hns@xxxxxxxxxxxxx> a écrit :
From: Paul Boddie <paul@xxxxxxxxxxxxx>
Add support for the LCD controller present on JZ4780 SoCs.
This SoC uses 8-byte descriptors which extend the current
4-byte descriptors used for other Ingenic SoCs.
Tested on MIPS Creator CI20 board.
Signed-off-by: Paul Boddie <paul@xxxxxxxxxxxxx>
Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
---
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85
+++++++++++++++++++++--
drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++++++++++
2 files changed, 122 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index f73522bdacaa..e2df4b085905 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -6,6 +6,7 @@
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