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

[Liu Ying]

On Thu, 2022-03-03 at 09:19 +0100, Lucas Stach wrote:
> Am Donnerstag, dem 03.03.2022 um 10:54 +0800 schrieb Liu Ying:
> > 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).
> 
> I'm not sure why you are so keen on using DRM leases in your
> downstream. Actually this argument is a argument in _favor_ of
> independent DRM devices: you don't need to deal with leases when every
> userspace component can just exclusively use a DRM device.

Userspace may choose to use drm lease APIs to talk to kernel. It's a
feature kinda nice to have. Imagine that an user has already written an
application which uses the APIs. 

> 
> > 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).
> 
> We already gather the GPU cores in etnaviv and as I said this decision
> proves to add complications in the long run. For example prime import
> with the DRM helpers is currently bound to the DRM device, so if your
> actual HW devices backing the DRM device have differing DMA constraints
> things get really messy.

LCDIFs in one SoC are very likely symmetric from the embodying system
PoV. So, maybe, that's not a big problem?

> 
> > 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.
> 
> It's not a big problem, but it adds complexity that wouldn't be there
> if you have a simple 1 IP instance <-> 1 DRM device mapping.

Just a little bit more complexity, I think. I tend to take that to
achieve better flexibilty and scalabilty(to support potential muxes).
It's a trade-off as I mentioned - I tend to choose one drm device, but
_no_ strong opinion on three.  Once the muxes become real, it looks
like we have to use the 'one drm device' solution.

> 
> > 4) Regarding the fake view of shared resources, atomic check can handle
> > that, so it doesn't seem to be a big problem, either.
> 
> Sure, there isn't even anything to handle, as the pipes are truly
> independent.
> 

Yes, but it's still a trade-off.