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@xxxxxxxxxxxxxx> 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.
