[PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output
Paul Cercueil
paul at crapouillou.net
Sun Nov 7 19:01:38 UTC 2021
Hi Nikolaus,
Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller
<hns at 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 at crapouillou.net>:
>>
>> Hi Nikolaus,
>>
>> Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller
>> <hns at goldelico.com> a écrit :
>>> From: Paul Boddie <paul at 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 at boddie.org.uk>
>>> Signed-off-by: Ezequiel Garcia <ezequiel at collabora.com>
>>> Signed-off-by: H. Nikolaus Schaller <hns at 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
>
More information about the dri-devel
mailing list