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@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:
- 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.
- 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?
- 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.
- 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.