lkml.org 
[lkml]   [2021]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 2/7] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Nikolaus,

Le dim., nov. 7 2021 at 21:25:38 +0100, H. Nikolaus Schaller
<hns@goldelico.com> a écrit :
> Hi Paul,
>
>> Am 07.11.2021 um 20:01 schrieb Paul Cercueil <paul@crapouillou.net>:
>>
>> Hi Nikolaus,
>>
>> Le dim., nov. 7 2021 at 14:41:18 +0100, H. Nikolaus Schaller
>> <hns@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@crapouillou.net>:
>>>> Hi Nikolaus,
>>>> Le mar., oct. 5 2021 at 14:29:14 +0200, H. Nikolaus Schaller
>>>> <hns@goldelico.com> a écrit :
>>>>> From: Paul Boddie <paul@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@boddie.org.uk>
>>>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@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;
>
> 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);
>>>> + &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...
>
>>
>> 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
>


\
 
 \ /
  Last update: 2021-11-08 10:38    [W:0.293 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site