Hi Nikolaus,
Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller
<hns@xxxxxxxxxxxxx> a écrit :
Hi Paul,
Am 07.11.2021 um 20:01 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
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;
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
