[PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Marek Vasut]

On 3/3/22 03:54, Liu Ying wrote:
> On Wed, 2022-03-02 at 12:57 +0100, Lucas Stach wrote:
>> Am Mittwoch, dem 02.03.2022 um 17:41 +0800 schrieb Liu Ying:
>>> On Wed, 2022-03-02 at 10:23 +0100, Lucas Stach wrote:
>>>> Am Mittwoch, dem 02.03.2022 um 03:54 +0100 schrieb Marek Vasut:
>>>>> On 3/1/22 14:18, Lucas Stach wrote:
>>>>>> Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford:
>>>>>>> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach at pengutronix.de> wrote:
>>>>>>>> Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut:
>>>>>>>>> On 3/1/22 11:04, Lucas Stach wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>>> Given the two totally different IPs, I don't see bugs of IP control
>>>>>>>>>>> logics should be fixed for both drivers. Naturally, the two would
>>>>>>>>>>> diverge due to different HWs. Looking at Patch 9/9, it basically
>>>>>>>>>>> squashes code to control LCDIFv3 into the mxsfb drm driver with
>>>>>>>>>>> 'if/else' checks(barely no common control code), which is hard to
>>>>>>>>>>> maintain and not able to achieve good scalability for both 'LCDIFv3'
>>>>>>>>>>> and 'LCDIF'.
>>>>>>>>>>
>>>>>>>>>> I tend to agree with Liu here. Writing a DRM driver isn't that much
>>>>>>>>>> boilerplate anymore with all the helpers we have available in the
>>>>>>>>>> framework today.
>>>>>>>>>
>>>>>>>>> I did write a separate driver for this IP before I spent time merging
>>>>>>>>> them into single driver, that's when I realized a single driver is much
>>>>>>>>> better and discarded the separate driver idea.
>>>>>>>>>
>>>>>>>>>> The IP is so different from the currently supported LCDIF controllers
>>>>>>>>>> that I think trying to support this one in the existing driver actually
>>>>>>>>>> increases the chances to break something when modifying the driver in
>>>>>>>>>> the future. Not everyone is able to test all LCDIF versions. My vote is
>>>>>>>>>> on having a separate driver for the i.MX8MP LCDIF.
>>>>>>>>>
>>>>>>>>> If you look at both controllers, it is clear it is still the LCDIF
>>>>>>>>> behind, even the CSC that is bolted on would suggest that.
>>>>>>>>
>>>>>>>> Yes, but from a driver PoV what you care about is not really the
>>>>>>>> hardware blocks used to implement something, but the programming model,
>>>>>>>> i.e. the register interface exposed to software.
>>>>>>>>
>>>>>>>>> I am also not happy when I look at the amount of duplication a separate
>>>>>>>>> driver would create, it will be some 50% of the code that would be just
>>>>>>>>> duplicated.
>>>>>>>>>
>>>>>>>> Yea, the duplicated code is still significant, as the HW itself is so
>>>>>>>> simple. However, if you find yourself in the situation where basically
>>>>>>>> every actual register access in the driver ends up being in a "if (some
>>>>>>>> HW rev) ... " clause, i still think it would be better to have a
>>>>>>>> separate driver, as the programming interface is just different.
>>>>>>>
>>>>>>> I tend to agree with Marek on this one.  We have an instance where the
>>>>>>> blk-ctrl and the GPC driver between 8m, mini, nano, plus are close,
>>>>>>> but different enough where each SoC has it's own set of tables and
>>>>>>> some checks.   Lucas created the framework, and others adapted it for
>>>>>>> various SoC's.  If there really is nearly 50% common code for the
>>>>>>> LCDIF, why not either leave the driver as one or split the common code
>>>>>>> into its own driver like lcdif-common and then have smaller drivers
>>>>>>> that handle their specific variations.
>>>>>>
>>>>>> I don't know exactly how the standalone driver looks like, but I guess
>>>>>> the overlap is not really in any real HW specific parts, but the common
>>>>>> DRM boilerplate, so there isn't much point in creating a common lcdif
>>>>>> driver.
>>>>>
>>>>> The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
>>>>> that, there is some 400 LoC which are specific to old LCDIF and this
>>>>> patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
>>>>> shared boilerplate that would be duplicated .
>>>>
>>>> That is probably ignoring the fact that the 8MP LCDIF does not support
>>>> any overlays, so it could use the drm_simple_display_pipe
>>>> infrastructure to reduce the needed boilerplate.
>>>
>>> The drm_simple_display_pipe infrastructure is probably too simple for
>>> i.MX8MP LCDIF, since it uses one only crtc for one drm device. i.MX8MP
>>> embeds *three* LCDIF instances to support MIPI DSI, LVDS and HDMI
>>> outputs respectively. To use that infrastructure means there would be
>>> three dri cards in all. However, the three LCDIF instances can be
>>> wrapped by the one drm device, which is not the boilerplate code in the
>>> current mxsfb driver may handle.
>>
>> While that may make things a little simpler for userspace, I'm not sure
>> if this is the right thing to do. It complicates the driver a lot,
>> especially if you want to get things like independent power management,
>> etc. right. It also creates a fake view for userspace, where is looks
>> like there might be some shared resources between the different display
>> paths, while in reality they are fully independent.
> 
> Trade-off will be made between one drm device and three. My first
> impression of using the drm_simple_display_pipe infrastructure is that
> it's too simple and less flexible/scalable, because SoC designer will
> be likely to add muxes between CRTCs and encoders/bridges or overlay
> plane(s) in next generations of SoCs(SW developers don't seem have good
> reasons to suggest not to do that).  Another concern is that whether
> the userspace may use the three drm devices well or not.
> 
> A few more points:
> 1) With one drm device, userspace may use drm lease APIs to control
> those independant pipes with drm masters(not sure about the userspace
> maturity).
> 2) Code to gather all LCDIFs as one drm device has chance to be created
> as helpers once there are similar use cases in other drivers(maybe,
> there is/are already).
> 3) Power management doesn't seem to be a problem, since each LCDIF has
> it's own struct device which can be used to do runtime PM at some
> drm_crtc_helper_funcs callbacks.
> 4) Regarding the fake view of shared resources, atomic check can handle
> that, so it doesn't seem to be a big problem, either.
> 
>>
>> While we do something similar on the GPU side and collect all GPU cores
>> under a single DRM device, I'm not fully convinced that this was a good
>> decision. It now comes back to bite us when the SoC topologies get a
>> little more interesting and e.g. devices are behind different IOMMU
>> streams.
> 
> Right, SoC topologies may change, like the aforementioned muxes.
> Generally speaking, I think one drm device is more flexible and
> scalable than three.

I agree with Lucas on one driver instance - one IP instance. Each IP 
instance is separate, so it should have separate driver instance bound 
to it.