Hi Nikolaus,

Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller 
[off-list ref] a écrit :

Hi Paul,

quoted

 Am 07.11.2021 um 20:01 schrieb Paul Cercueil [off-list ref]:

 Hi Nikolaus,

 Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller 
[off-list ref] a écrit :

quoted

 Hi Paul,
 sorry for the delay in getting back to this, but I was distracted 
by more urgent topics.

quoted

 Am 05.10.2021 um 22:22 schrieb Paul Cercueil 
[off-list ref]:
 Hi Nikolaus,
 Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller 
[off-list ref] a écrit :

quoted

 From: Paul Boddie [off-list ref]
 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 [off-list ref]
 Signed-off-by: Ezequiel Garcia [off-list ref]
 Signed-off-by: H. Nikolaus Schaller [off-list ref]
 ---
 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.

quoted

 That's enough to get an image.

Fine that we can agree on that.

quoted

quoted

 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.

quoted

quoted

 	}
 +	regmap_config = ingenic_drm_regmap_config;
 +	regmap_config.max_register = soc_info->max_reg;
 	priv->map = devm_regmap_init_mmio(dev, base,
 -					  &ingenic_drm_regmap_config);
 +					  &regmap_config);
 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...

quoted

 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.

quoted

quoted

 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.

quoted

quoted

 So I leave it in v6 unsplitted.

quoted

quoted

 	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...

quoted

quoted

 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.

quoted

quoted

quoted

quoted

 +	}

 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.

quoted

 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

quoted

quoted

 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