Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On 3/3/22 09:21, Lucas Stach wrote: Am Donnerstag, dem 03.03.2022 um 04:14 +0100 schrieb Marek Vasut: On 3/2/22 10:23, Lucas Stach wrote: [...] 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. It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, the mxsfb and hypothetical lcdif drivers would look really very similar. As you brought up the blk-ctrl as an example: I'm all for supporting slightly different hardware in the same driver, as long as the HW interface is close enough. But then I also opted for a separate 8MP blk-ctrl driver for those blk-ctrls that differ significantly from the others, as I think it would make the common driver unmaintainable trying to support all the different variants in one driver. But then you also need to maintain two sets of boilerplate, they diverge, and that is not good. I don't think that there is much chance for bugs going unfixed due to divergence in the boilerplate, especially if you use the simple pipe framework to handle most of that stuff for you, which gives you a lot of code sharing with other simple DRM drivers. But I can not use the simple pipe because overlays, see imxrt1070 . [...] We can always split the drivers later if this becomes unmaintainable too, no ? Not if you want to keep the same userspace running. As userspace has some ties to the DRM driver name, e.g. for finding the right GBM implementation, splitting the driver later on would be a UABI break. Hum, so what other options do we have left ? Duplicate 60% of the driver right away ?
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 > > > > > > > > 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
RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
>-Original Message- >From: Marek Vasut >Sent: 2022年3月2日 10:50 >To: Robby Cai ; Lucas Stach ; >Adam Ford >Cc: Ying Liu (OSS) ; dri-devel >; devicetree ; >Peng Fan ; Alexander Stein >; Rob Herring ; >Laurent Pinchart ; Sam Ravnborg > >Subject: Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for >i.MX8MP > >Caution: EXT Email > >On 3/1/22 14:37, Robby Cai wrote: > >Hi, > >[...] > >>>> 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. >>> >>> As you brought up the blk-ctrl as an example: I'm all for supporting >>> slightly different hardware in the same driver, as long as the HW >>> interface is close enough. But then I also opted for a separate 8MP >>> blk-ctrl driver for those blk-ctrls that differ significantly from >>> the others, as I think it would make the common driver unmaintainable >>> trying to support all the different variants in one driver. >>> >>> Regards, >>> Lucas >> >> LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, >although it's also called 'LCDIF'. >> We prefer not mix these two series of IPs in one driver for ease of >maintenance and extension. > >Where does the MX8MP LCDIF come from then, SGTL maybe ? AFAIK, it's RT1170. You may have a check on RM [1]. Interestingly, this SoC has both eLCDIF and LCDIFv2, two IPs we are talking about. [1] https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM Regards, Robby
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Am Donnerstag, dem 03.03.2022 um 04:14 +0100 schrieb Marek Vasut: > On 3/2/22 10:23, Lucas Stach wrote: > > [...] > > > > > > 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. > > It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, > the mxsfb and hypothetical lcdif drivers would look really very similar. > > > > > As you brought up the blk-ctrl as an example: I'm all for supporting > > > > slightly different hardware in the same driver, as long as the HW > > > > interface is close enough. But then I also opted for a separate 8MP > > > > blk-ctrl driver for those blk-ctrls that differ significantly from the > > > > others, as I think it would make the common driver unmaintainable > > > > trying to support all the different variants in one driver. > > > > > > But then you also need to maintain two sets of boilerplate, they > > > diverge, and that is not good. > > > > I don't think that there is much chance for bugs going unfixed due to > > divergence in the boilerplate, especially if you use the simple pipe > > framework to handle most of that stuff for you, which gives you a lot > > of code sharing with other simple DRM drivers. > > But I can not use the simple pipe because overlays, see imxrt1070 . > > [...] > > We can always split the drivers later if this becomes unmaintainable > too, no ? Not if you want to keep the same userspace running. As userspace has some ties to the DRM driver name, e.g. for finding the right GBM implementation, splitting the driver later on would be a UABI break. Regards, Lucas
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 > > > > > > > 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 an
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 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 dev
Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On 3/2/22 14:14, Robby Cai wrote: Hi [...] LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'. We prefer not mix these two series of IPs in one driver for ease of maintenance and extension. Where does the MX8MP LCDIF come from then, SGTL maybe ? AFAIK, it's RT1170. You may have a check on RM [1]. Interestingly, this SoC has both eLCDIF and LCDIFv2, two IPs we are talking about. [1] https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM That's interesting, I wasn't aware of this one. So that's MX8MP LCDIF + overlays. Did the LCDIF v2 in iMXRT1070 have any predecessor too ?
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On 3/2/22 10:23, Lucas Stach wrote: [...] 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. It seems the IMXRT1070 LCDIF v2 (heh ...) does support overlays, so no, the mxsfb and hypothetical lcdif drivers would look really very similar. As you brought up the blk-ctrl as an example: I'm all for supporting slightly different hardware in the same driver, as long as the HW interface is close enough. But then I also opted for a separate 8MP blk-ctrl driver for those blk-ctrls that differ significantly from the others, as I think it would make the common driver unmaintainable trying to support all the different variants in one driver. But then you also need to maintain two sets of boilerplate, they diverge, and that is not good. I don't think that there is much chance for bugs going unfixed due to divergence in the boilerplate, especially if you use the simple pipe framework to handle most of that stuff for you, which gives you a lot of code sharing with other simple DRM drivers. But I can not use the simple pipe because overlays, see imxrt1070 . [...] We can always split the drivers later if this becomes unmaintainable too, no ?
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 > > > > > > 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
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 > > > > > 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
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 > > > > 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. Regards, Liu Ying > > > As you brought up the blk-ctrl as an example: I'm all for supporting > > > slightly different hardware in the same driver, a
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 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. > > > As you brought up the blk-ctrl as an example: I'm all for supporting > > slightly different hardware in the same driver, as long as the HW > > interface is close enough. But then I also opted for a separate 8MP > > blk-ctrl driver for those blk-ctrls that differ significantly from the > > others, as I think it would make the common driver unmaintainable > > trying to support all the different variants in one driver. > > But then you also need to maintain two sets of boilerplate, they > diverge, and that is not good. I don't think that there is much chance for bugs going unfixed due to divergence in the boilerplate, especially if you use the simple pipe framework to handle most of that stuff for you, which gives you a lot of code sharing with other simple DRM drivers. Regards, Lucas
RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
>-Original Message- >From: Lucas Stach >Sent: 2022年3月1日 21:19 >To: Adam Ford >Cc: Marek Vasut ; Ying Liu (OSS) ; >dri-devel ; devicetree >; Peng Fan ; Alexander Stein >; Rob Herring ; >Laurent Pinchart ; Sam Ravnborg >; Robby Cai >Subject: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for >i.MX8MP > >Caution: EXT Email > >Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford: >> On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach >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. > >As you brought up the blk-ctrl as an example: I'm all for supporting slightly >different hardware in the same driver, as long as the HW interface is close >enough. But then I also opted for a separate 8MP blk-ctrl driver for those >blk-ctrls that differ significantly from the others, as I think it would make >the >common driver unmaintainable trying to support all the different variants in >one driver. > >Regards, >Lucas LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'. We prefer not mix these two series of IPs in one driver for ease of maintenance and extension. Regards, Robby
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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 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 . As you brought up the blk-ctrl as an example: I'm all for supporting slightly different hardware in the same driver, as long as the HW interface is close enough. But then I also opted for a separate 8MP blk-ctrl driver for those blk-ctrls that differ significantly from the others, as I think it would make the common driver unmaintainable trying to support all the different variants in one driver. But then you also need to maintain two sets of boilerplate, they diverge, and that is not good.
Re: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On 3/1/22 14:37, Robby Cai wrote: Hi, [...] 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. As you brought up the blk-ctrl as an example: I'm all for supporting slightly different hardware in the same driver, as long as the HW interface is close enough. But then I also opted for a separate 8MP blk-ctrl driver for those blk-ctrls that differ significantly from the others, as I think it would make the common driver unmaintainable trying to support all the different variants in one driver. Regards, Lucas LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'. We prefer not mix these two series of IPs in one driver for ease of maintenance and extension. Where does the MX8MP LCDIF come from then, SGTL maybe ?
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford: > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach 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. As you brought up the blk-ctrl as an example: I'm all for supporting slightly different hardware in the same driver, as long as the HW interface is close enough. But then I also opted for a separate 8MP blk-ctrl driver for those blk-ctrls that differ significantly from the others, as I think it would make the common driver unmaintainable trying to support all the different variants in one driver. Regards, Lucas
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach 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. adam > > Regards, > Lucas > >
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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. Regards, Lucas
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
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. 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. [...]
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Hi Marek, hi Liu, Am Dienstag, dem 01.03.2022 um 10:44 +0800 schrieb Liu Ying: > On Mon, 2022-02-28 at 16:34 +0100, Marek Vasut wrote: > > On 2/28/22 09:18, Liu Ying wrote: > > > > Hi, > > Hi, > > > > > > > > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote: > > > > > > Add compatible string for i.MX8MP LCDIF variant. This is called > > > > > > LCDIFv3 > > > > > > and is completely different from the LCDIFv3 found in i.MX23 in > > > > > > that it > > > > > > > > > > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF. > > > > > > > > See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF > > > > V4 . > > > > > > Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display > > > controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'. There > > > is not even a register in i.MX8MP display controller to decribe the > > > version. > > > > We also don't have a version register on MX6SX and we call it LCDIF V6 > > in the driver. The naming scheme is confusing. > > It looks ok for the current mxsfb drm driver to use its own version > tracking mechanism to distinguish kinda small difference across LCDIF > variants. However, LCDIFv3 in i.MX8mp is a totally different IP, which > does not apply to the tracking mechanism. > > > > > > > > > has a completely scrambled register layout compared to all previous > > > > > > LCDIF > > > > > > > > > > It looks like no single register of i.MX8MP LCDIFv3 overlaps with > > > > > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram > > > > > is > > > > > totally different from the LCDIF block diagram, according to the SoC > > > > > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal > > > > > and vertical size of graphic, position of graphic on the panel, > > > > > address > > > > > of graphic in memory and color formats or color palettes, which is not > > > > > supported by LCDIF and impacts display driver control mechanism > > > > > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC > > > > > interface, while LCDIFv3 only supports parallel output as a > > > > > counterpart > > > > > of the DOTCLK interface. > > > > > > > > > > Generally speaking, LCDIFv3 is just a new display IP which happens to > > > > > have the word 'LCDIF' in its name. Although both of LCDIFv3 and LCDIF > > > > > are display controllers for scanning out frames onto display devices, > > > > > I > > > > > don't think they are in one family. > > > > > > > > > > So, LCDIFv3 deserves a new separate dt-binding, IMO. > > > > > > > > It seems to me a lot of those bits just map to their previous > > > > equivalents in older LCDIF, others were dropped, so this is some sort of > > > > new LCDIF mutation, is it not ? > > > > > > I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names > > > of registers and the names of register bits . > > > > > > > I am aware NXP has a separate driver in its downstream, but I'm not > > > > convinced the duplication of boilerplate code by introducing a separate > > > > driver for what looks like another LCDIF variant is the right approach. > > > > > > Hmmm, given the two IPs, I think there should be separate drivers. > > > With one single driver, there would be too many 'if/else' checks to > > > separate the logics for the IPs, just like Patch 9/9 does. The > > > boilerplate code to do things like registering a drm device is > > > acceptable, IMO. > > > > > > Aside from that, with separate drivers, we don't have to test too many > > > SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. > > > > But then, with two drivers, you also might miss fixes which get applied > > to one driver and not the other, eventually the two drivers will diverge > > and that's not good. > > 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. 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. Regards, Lucas
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On Mon, 2022-02-28 at 16:34 +0100, Marek Vasut wrote: > On 2/28/22 09:18, Liu Ying wrote: > > Hi, Hi, > > > > > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote: > > > > > Add compatible string for i.MX8MP LCDIF variant. This is called > > > > > LCDIFv3 > > > > > and is completely different from the LCDIFv3 found in i.MX23 in that > > > > > it > > > > > > > > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF. > > > > > > See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF > > > V4 . > > > > Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display > > controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'. There > > is not even a register in i.MX8MP display controller to decribe the > > version. > > We also don't have a version register on MX6SX and we call it LCDIF V6 > in the driver. The naming scheme is confusing. It looks ok for the current mxsfb drm driver to use its own version tracking mechanism to distinguish kinda small difference across LCDIF variants. However, LCDIFv3 in i.MX8mp is a totally different IP, which does not apply to the tracking mechanism. > > > > > > has a completely scrambled register layout compared to all previous > > > > > LCDIF > > > > > > > > It looks like no single register of i.MX8MP LCDIFv3 overlaps with > > > > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is > > > > totally different from the LCDIF block diagram, according to the SoC > > > > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal > > > > and vertical size of graphic, position of graphic on the panel, address > > > > of graphic in memory and color formats or color palettes, which is not > > > > supported by LCDIF and impacts display driver control mechanism > > > > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC > > > > interface, while LCDIFv3 only supports parallel output as a counterpart > > > > of the DOTCLK interface. > > > > > > > > Generally speaking, LCDIFv3 is just a new display IP which happens to > > > > have the word 'LCDIF' in its name. Although both of LCDIFv3 and LCDIF > > > > are display controllers for scanning out frames onto display devices, I > > > > don't think they are in one family. > > > > > > > > So, LCDIFv3 deserves a new separate dt-binding, IMO. > > > > > > It seems to me a lot of those bits just map to their previous > > > equivalents in older LCDIF, others were dropped, so this is some sort of > > > new LCDIF mutation, is it not ? > > > > I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names > > of registers and the names of register bits . > > > > > I am aware NXP has a separate driver in its downstream, but I'm not > > > convinced the duplication of boilerplate code by introducing a separate > > > driver for what looks like another LCDIF variant is the right approach. > > > > Hmmm, given the two IPs, I think there should be separate drivers. > > With one single driver, there would be too many 'if/else' checks to > > separate the logics for the IPs, just like Patch 9/9 does. The > > boilerplate code to do things like registering a drm device is > > acceptable, IMO. > > > > Aside from that, with separate drivers, we don't have to test too many > > SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. > > But then, with two drivers, you also might miss fixes which get applied > to one driver and not the other, eventually the two drivers will diverge > and that's not good. 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 might wait for opinion from the others whether this should be one or > two drivers. > > btw is there any plan to have LCDIFv4 or this LCDIFv3 in some other SoC > than iMX8MP ? I don't think this kind of information significantly impacts the decision on two drivers or one. And, I cannot share that until the company unveils.
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On 2/28/22 09:18, Liu Ying wrote: Hi, On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote: Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3 and is completely different from the LCDIFv3 found in i.MX23 in that it In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF. See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF V4 . Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'. There is not even a register in i.MX8MP display controller to decribe the version. We also don't have a version register on MX6SX and we call it LCDIF V6 in the driver. The naming scheme is confusing. has a completely scrambled register layout compared to all previous LCDIF It looks like no single register of i.MX8MP LCDIFv3 overlaps with registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is totally different from the LCDIF block diagram, according to the SoC reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal and vertical size of graphic, position of graphic on the panel, address of graphic in memory and color formats or color palettes, which is not supported by LCDIF and impacts display driver control mechanism considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC interface, while LCDIFv3 only supports parallel output as a counterpart of the DOTCLK interface. Generally speaking, LCDIFv3 is just a new display IP which happens to have the word 'LCDIF' in its name. Although both of LCDIFv3 and LCDIF are display controllers for scanning out frames onto display devices, I don't think they are in one family. So, LCDIFv3 deserves a new separate dt-binding, IMO. It seems to me a lot of those bits just map to their previous equivalents in older LCDIF, others were dropped, so this is some sort of new LCDIF mutation, is it not ? I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names of registers and the names of register bits . I am aware NXP has a separate driver in its downstream, but I'm not convinced the duplication of boilerplate code by introducing a separate driver for what looks like another LCDIF variant is the right approach. Hmmm, given the two IPs, I think there should be separate drivers. With one single driver, there would be too many 'if/else' checks to separate the logics for the IPs, just like Patch 9/9 does. The boilerplate code to do things like registering a drm device is acceptable, IMO. Aside from that, with separate drivers, we don't have to test too many SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. But then, with two drivers, you also might miss fixes which get applied to one driver and not the other, eventually the two drivers will diverge and that's not good. I might wait for opinion from the others whether this should be one or two drivers. btw is there any plan to have LCDIFv4 or this LCDIFv3 in some other SoC than iMX8MP ?
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On Mon, 2022-02-28 at 07:57 +0100, Marek Vasut wrote: > On 2/28/22 07:37, Liu Ying wrote: > > Hi Marek, > > Hi, > > > On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote: > > > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3 > > > and is completely different from the LCDIFv3 found in i.MX23 in that it > > > > In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF. > > See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF > V4 . Ok, got it now. AFAIK, the SoC design team calls i.MX8MP display controller as 'LCDIFv3'. Those in other SoCs are called 'LCDIF'. There is not even a register in i.MX8MP display controller to decribe the version. > > > > has a completely scrambled register layout compared to all previous LCDIF > > > > It looks like no single register of i.MX8MP LCDIFv3 overlaps with > > registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is > > totally different from the LCDIF block diagram, according to the SoC > > reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal > > and vertical size of graphic, position of graphic on the panel, address > > of graphic in memory and color formats or color palettes, which is not > > supported by LCDIF and impacts display driver control mechanism > > considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC > > interface, while LCDIFv3 only supports parallel output as a counterpart > > of the DOTCLK interface. > > > > Generally speaking, LCDIFv3 is just a new display IP which happens to > > have the word 'LCDIF' in its name. Although both of LCDIFv3 and LCDIF > > are display controllers for scanning out frames onto display devices, I > > don't think they are in one family. > > > > So, LCDIFv3 deserves a new separate dt-binding, IMO. > > It seems to me a lot of those bits just map to their previous > equivalents in older LCDIF, others were dropped, so this is some sort of > new LCDIF mutation, is it not ? I say 'LCDIFv3' and 'LCDIF' are totally two IPs, if I compare the names of registers and the names of register bits . > > I am aware NXP has a separate driver in its downstream, but I'm not > convinced the duplication of boilerplate code by introducing a separate > driver for what looks like another LCDIF variant is the right approach. Hmmm, given the two IPs, I think there should be separate drivers. With one single driver, there would be too many 'if/else' checks to separate the logics for the IPs, just like Patch 9/9 does. The boilerplate code to do things like registering a drm device is acceptable, IMO. Aside from that, with separate drivers, we don't have to test too many SoCs if we only want to touch either 'LCDIFv3' or 'LCDIF'. > > > > variants. The new LCDIFv3 also supports 36bit address space. However, > > > except for the complete bit reshuffling, this is still LCDIF and it still > > > works like one. > > [...]
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
On 2/28/22 07:37, Liu Ying wrote: Hi Marek, Hi, On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote: Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3 and is completely different from the LCDIFv3 found in i.MX23 in that it In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF. See i.MX23 HW_LCDIF_VERSION MAJOR=0x3 , that's LCDIF V3 . MX28 has LCDIF V4 . has a completely scrambled register layout compared to all previous LCDIF It looks like no single register of i.MX8MP LCDIFv3 overlaps with registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is totally different from the LCDIF block diagram, according to the SoC reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal and vertical size of graphic, position of graphic on the panel, address of graphic in memory and color formats or color palettes, which is not supported by LCDIF and impacts display driver control mechanism considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC interface, while LCDIFv3 only supports parallel output as a counterpart of the DOTCLK interface. Generally speaking, LCDIFv3 is just a new display IP which happens to have the word 'LCDIF' in its name. Although both of LCDIFv3 and LCDIF are display controllers for scanning out frames onto display devices, I don't think they are in one family. So, LCDIFv3 deserves a new separate dt-binding, IMO. It seems to me a lot of those bits just map to their previous equivalents in older LCDIF, others were dropped, so this is some sort of new LCDIF mutation, is it not ? I am aware NXP has a separate driver in its downstream, but I'm not convinced the duplication of boilerplate code by introducing a separate driver for what looks like another LCDIF variant is the right approach. variants. The new LCDIFv3 also supports 36bit address space. However, except for the complete bit reshuffling, this is still LCDIF and it still works like one. [...]
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Hi Marek, Thank you for the patch. On Mon, Feb 28, 2022 at 01:45:57AM +0100, Marek Vasut wrote: > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3 > and is completely different from the LCDIFv3 found in i.MX23 in that it > has a completely scrambled register layout compared to all previous LCDIF > variants. The new LCDIFv3 also supports 36bit address space. However, > except for the complete bit reshuffling, this is still LCDIF and it still > works like one. > > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Rob Herring > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner > Cc: devicet...@vger.kernel.org > --- > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > index 900a56cae80e6..9831ab53a053d 100644 > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > @@ -28,6 +28,7 @@ properties: >- fsl,imx7d-lcdif >- fsl,imx8mm-lcdif >- fsl,imx8mn-lcdif > + - fsl,imx8mp-lcdif As the hardware isn't backward-compatible with any other version, I think the new compatible string should go in the previous enum block, not in this one. We don't want the imx6sx fallback. >- fsl,imx8mq-lcdif >- const: fsl,imx6sx-lcdif > -- Regards, Laurent Pinchart
Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Hi Marek, On Mon, 2022-02-28 at 01:45 +0100, Marek Vasut wrote: > Add compatible string for i.MX8MP LCDIF variant. This is called LCDIFv3 > and is completely different from the LCDIFv3 found in i.MX23 in that it In i.MX23 reference manual, there is no LCDIFv3 found, but only LCDIF. > has a completely scrambled register layout compared to all previous LCDIF It looks like no single register of i.MX8MP LCDIFv3 overlaps with registers in other i.MX2x/6x/7x/8x LCDIFs. The LCDIFv3 block diagram is totally different from the LCDIF block diagram, according to the SoC reference manuals. LCDIFv3 supports SHADOW_EN bit to update horizontal and vertical size of graphic, position of graphic on the panel, address of graphic in memory and color formats or color palettes, which is not supported by LCDIF and impacts display driver control mechanism considerably. LCDIF supports DOTCLK interface, MPU interface and VSYNC interface, while LCDIFv3 only supports parallel output as a counterpart of the DOTCLK interface. Generally speaking, LCDIFv3 is just a new display IP which happens to have the word 'LCDIF' in its name. Although both of LCDIFv3 and LCDIF are display controllers for scanning out frames onto display devices, I don't think they are in one family. So, LCDIFv3 deserves a new separate dt-binding, IMO. > variants. The new LCDIFv3 also supports 36bit address space. However, > except for the complete bit reshuffling, this is still LCDIF and it still > works like one. > > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Rob Herring > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner > Cc: devicet...@vger.kernel.org > --- > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > index 900a56cae80e6..9831ab53a053d 100644 > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > @@ -28,6 +28,7 @@ properties: >- fsl,imx7d-lcdif >- fsl,imx8mm-lcdif >- fsl,imx8mn-lcdif > + - fsl,imx8mp-lcdif Even if LCDIFv3 is covered by this dt-binding(which is obviously not the case), 'fsl,imx8mp-lcdif' should be after 'fsl,imx6x-lcdif' as an enum, otherwise LCDIFv3 is compatible to LCDIF. Regards, Liu Ying >- fsl,imx8mq-lcdif >- const: fsl,imx6sx-lcdif >
