by Paul Boddie

[permalink] [raw]

Subject: Re: [PATCH v12 2/9] drm/ingenic: Add support for JZ4780 and HDMI output

On Wednesday, 2 February 2022 13:41:21 CET Paul Cercueil wrote:
> Le mer., f?vr. 2 2022 at 13:33:15 +0100, H. Nikolaus Schaller>
<[email protected]> a ?crit :
> >> Am 02.02.2022 um 13:28 schrieb Paul Cercueil <[email protected]>:
> >>
> >> Note that there are other things not working with your current
> >>
> >> implementation, for instance you cannot set the X/Y start position
> >> of the f1 plane, which means it's only really usable for fullscreen
> >> desktop/windows.
> >
> > Is setting x/y possible for the other SoC?
>
> Yes. They support different x/y positions, sizes, and pixel format for
> both f0, f1 and IPU planes.

One thing worth noting about the JZ4780 is that a lot of the registers that
might otherwise be used for the above purposes appear to be read-only, at
least for the different fields concerned.

For example, those affecting ingenic_drm_plane_config:

Control Register (LCDCTRL) - specifically the BPP0 field
OSD Control Register (LCDOSDCTRL)
Foreground 0 XY Position Register (LCDXYP0)
Foreground 1 XY Position Register (LCDXYP1)
Foreground 0 Size Register (LCDSIZE0)
Foreground 1 Size Register (LCDSIZE1)

These require changes to the extended descriptor members instead, and I am
fairly sure I mentioned the implications for pixel depth configuration
previously. So, as far as I can tell, we would need to update the descriptors,
not the registers, to support the operations mentioned above.

As for the f0 plane "not working", I am not aware of any limitation around
using only f0 (assuming it corresponds to what the manual calls fg0) or only
f1 (again, assuming fg1 in the manual) or both. My assumption was that for
this particular driver, f0 was reserved for some kind of overlay and that f1
was to be used for the normal non-overlay display for products where the OSD
peripheral is provided.

From the definition of struct ingenic_drm:

/*
* f1 (aka. foreground1) is our primary plane, on top of which
* f0 (aka. foreground0) can be overlayed. Z-order is fixed in
* hardware and cannot be changed.
*/

So, as I understood it, the driver would configure f1 in the case of the
JZ4780 for basic display support. Configuring f0 as an overlay should be
entirely possible, but I imagine that it needs to change the descriptors, not
the registers, to have a chance of actually working.

I hope this is somewhat useful information. I honestly don't know if, say, the
JZ4770 has a similar arrangement with regard to configuration via descriptors,
as opposed to registers, but I think it is an important distinction between
devices in this particular family that needs to be accommodated in the driver,
and we obviously want to determine how this might best be achieved.

Paul

2022-02-05 18:59:24

by Paul Cercueil

[permalink] [raw]

Subject: Re: [PATCH v12 2/9] drm/ingenic: Add support for JZ4780 and HDMI output

Le mer., f?vr. 2 2022 at 13:33:15 +0100, H. Nikolaus Schaller
<[email protected]> a ?crit :
>
>
>> Am 02.02.2022 um 13:28 schrieb Paul Cercueil <[email protected]>:
>>
>>
>>
>> Le mer., f?vr. 2 2022 at 13:17:14 +0100, H. Nikolaus Schaller
>> <[email protected]> a ?crit :
>>> Hi Paul,
>>>> Am 02.02.2022 um 13:06 schrieb Paul Cercueil
>>>> <[email protected]>:
>>>> Hi Nikolaus,
>>>>>>> @@ -446,6 +454,9 @@ static int
>>>>>>> ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>>>>>>> if (!crtc)
>>>>>>> return 0;
>>>>>>> + if (plane == &priv->f0)
>>>>>>> + return -EINVAL;
>>>>>> This will break JZ4725B -> JZ4770 SoCs, the f0 plane is
>>>>>> perfectly usable there.
>>>>> Hm. I think it was your request/proposal to add this [1]?
>>>> Because otherwise with your current patchset the f0 plane does
>>>> not work *on JZ4780*.
>>> Not that I am eager to fix that, but...
>>> maybe it could be better to fix than having the check and -EINVAL
>>> depend on SoC compatible string
>>> (or some new flag in soc_info. plane_f0_not_working)?
>>
>> Totally agree! A proper fix would be much better. A
>> "plane_f0_not_working" in the meantime is OK with me.
>
> Ok, then I'll prepare a v13 with plane_f0_not_working.
>
>>
>> Note that there are other things not working with your current
>> implementation, for instance you cannot set the X/Y start position
>> of the f1 plane, which means it's only really usable for fullscreen
>> desktop/windows.
>
> Is setting x/y possible for the other SoC?

Yes. They support different x/y positions, sizes, and pixel format for
both f0, f1 and IPU planes.

-Paul

>>
>>>> It does work on older SoCs.
>>>>> What I have forgotten is why the f0 plane should not be usable
>>>>> for jz4780.
>>>> We return an error here to prevent userspace from using the f0
>>>> plane until it's effectively working on the JZ4780.
>>> Well, what would be not working with that plane if user-space
>>> would try to use it?
>>
>> From what I remember, it wouldn't show anything on screen, and
>> after that trying to use the f1 plane wouldn't work either.
>
> Ok. That may become a big project to fix. So let's do step 1 first.
>
> BR and thanks,
> NIkolaus
>