From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7082BC433F5 for ; Thu, 3 Mar 2022 04:14:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229557AbiCCEPQ (ORCPT ); Wed, 2 Mar 2022 23:15:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33284 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229470AbiCCEPP (ORCPT ); Wed, 2 Mar 2022 23:15:15 -0500 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5AE3DF4D38 for ; Wed, 2 Mar 2022 20:14:28 -0800 (PST) Received: from [127.0.0.1] (p578adb1c.dip0.t-ipconnect.de [87.138.219.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: marex@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id 8919F83B9F; Thu, 3 Mar 2022 05:14:25 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1646280866; bh=2xWG+M4NrZBWRPXzWMdjfDGDaYcG7GwtQKJiX8d2DEk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=MXWpKZrPKXfgBcrp1gNoymzGBVfzz8+gn74WyjQDumcBNJp6gYNy+RAHOxiQEd9w6 ryIoB4fcch+BbLzpBIlGgKryD23piaYQU/OAsP9CsptpwgVEPVpsBOnrnr+OR8VF/c f1zWNGJAYfkc8LyarPMuNMAnkgoSyzI+Rq4avzLr1Z5Xa9+JhERBBEFbYGjEeJ3Z7j jwTn/mYkBl2IijaT93E20clYmLe5MHJouCvOpeapc8qDdoWtR/tqKU5ocvKSir4w45 gIMSOC/vZCBBfx3VsrPXMT2fb+gljvCAzVF9fLvpbhRJHPkjVLG7opzzt6E3UORHQ9 FPivVfz5G99iQ== Message-ID: Date: Thu, 3 Mar 2022 05:14:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP Content-Language: en-US To: Liu Ying , Lucas Stach , Adam Ford Cc: dri-devel , devicetree , Peng Fan , Alexander Stein , Rob Herring , Laurent Pinchart , Sam Ravnborg , Robby Cai References: <20220228004605.367040-1-marex@denx.de> <35b981d0d9d763525c427491ca0e25b6e4c03d0f.camel@oss.nxp.com> <8eac8a2c-bc6d-0c79-c727-bdaa2cd9abee@denx.de> <33207e88-da9b-96d7-0fef-461cb4496c88@denx.de> <284d65f53dffb6085bde6ef6ecd398f10d4c6c80.camel@oss.nxp.com> <8950434843ff7bbd1a527b0c799d9a74a75ee36d.camel@pengutronix.de> <7aeed693-dfb7-950f-fdf0-3c90de285392@denx.de> <8bf0b5a1c9ab9faee28077436cdfd49c0cd08792.camel@pengutronix.de> <4253aa4b5dc4a3568e45755678849961468bfd38.camel@pengutronix.de> <85af7c5dfa120903a22e5e704e3bddd87830033c.camel@pengutronix.de> <049a182d8bf75110dc5ebe72f5b58d209b64d58a.camel@oss.nxp.com> <7e0323b120ebd8faef162a9b0f0ab048bdb7a34b.camel@pengutronix.de> <15a6222b256f67a01ef947bb44a2737a6606ad4c.camel@oss.nxp.com> From: Marek Vasut In-Reply-To: <15a6222b256f67a01ef947bb44a2737a6606ad4c.camel@oss.nxp.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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 device, userspace may use drm lease APIs to control > those independant pipes with drm masters(not sure about the userspace > maturity). > 2) Code to gather all LCDIFs as one drm device has chance to be created > as helpers once there are similar use cases in other drivers(maybe, > there is/are already). > 3) Power management doesn't seem to be a problem, since each LCDIF has > it's own struct device which can be used to do runtime PM at some > drm_crtc_helper_funcs callbacks. > 4) Regarding the fake view of shared resources, atomic check can handle > that, so it doesn't seem to be a big problem, either. > >> >> While we do something similar on the GPU side and collect all GPU cores >> under a single DRM device, I'm not fully convinced that this was a good >> decision. It now comes back to bite us when the SoC topologies get a >> little more interesting and e.g. devices are behind different IOMMU >> streams. > > Right, SoC topologies may change, like the aforementioned muxes. > Generally speaking, I think one drm device is more flexible and > scalable than three. I agree with Lucas on one driver instance - one IP instance. Each IP instance is separate, so it should have separate driver instance bound to it. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EEFB4C433F5 for ; Thu, 3 Mar 2022 04:14:30 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0608410E61D; Thu, 3 Mar 2022 04:14:30 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [IPv6:2a01:238:438b:c500:173d:9f52:ddab:ee01]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7EB8810E61D for ; Thu, 3 Mar 2022 04:14:28 +0000 (UTC) Received: from [127.0.0.1] (p578adb1c.dip0.t-ipconnect.de [87.138.219.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: marex@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id 8919F83B9F; Thu, 3 Mar 2022 05:14:25 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1646280866; bh=2xWG+M4NrZBWRPXzWMdjfDGDaYcG7GwtQKJiX8d2DEk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=MXWpKZrPKXfgBcrp1gNoymzGBVfzz8+gn74WyjQDumcBNJp6gYNy+RAHOxiQEd9w6 ryIoB4fcch+BbLzpBIlGgKryD23piaYQU/OAsP9CsptpwgVEPVpsBOnrnr+OR8VF/c f1zWNGJAYfkc8LyarPMuNMAnkgoSyzI+Rq4avzLr1Z5Xa9+JhERBBEFbYGjEeJ3Z7j jwTn/mYkBl2IijaT93E20clYmLe5MHJouCvOpeapc8qDdoWtR/tqKU5ocvKSir4w45 gIMSOC/vZCBBfx3VsrPXMT2fb+gljvCAzVF9fLvpbhRJHPkjVLG7opzzt6E3UORHQ9 FPivVfz5G99iQ== Message-ID: Date: Thu, 3 Mar 2022 05:14:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP Content-Language: en-US To: Liu Ying , Lucas Stach , Adam Ford References: <20220228004605.367040-1-marex@denx.de> <35b981d0d9d763525c427491ca0e25b6e4c03d0f.camel@oss.nxp.com> <8eac8a2c-bc6d-0c79-c727-bdaa2cd9abee@denx.de> <33207e88-da9b-96d7-0fef-461cb4496c88@denx.de> <284d65f53dffb6085bde6ef6ecd398f10d4c6c80.camel@oss.nxp.com> <8950434843ff7bbd1a527b0c799d9a74a75ee36d.camel@pengutronix.de> <7aeed693-dfb7-950f-fdf0-3c90de285392@denx.de> <8bf0b5a1c9ab9faee28077436cdfd49c0cd08792.camel@pengutronix.de> <4253aa4b5dc4a3568e45755678849961468bfd38.camel@pengutronix.de> <85af7c5dfa120903a22e5e704e3bddd87830033c.camel@pengutronix.de> <049a182d8bf75110dc5ebe72f5b58d209b64d58a.camel@oss.nxp.com> <7e0323b120ebd8faef162a9b0f0ab048bdb7a34b.camel@pengutronix.de> <15a6222b256f67a01ef947bb44a2737a6606ad4c.camel@oss.nxp.com> From: Marek Vasut In-Reply-To: <15a6222b256f67a01ef947bb44a2737a6606ad4c.camel@oss.nxp.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree , Peng Fan , Alexander Stein , dri-devel , Rob Herring , Laurent Pinchart , Sam Ravnborg , Robby Cai Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 device, userspace may use drm lease APIs to control > those independant pipes with drm masters(not sure about the userspace > maturity). > 2) Code to gather all LCDIFs as one drm device has chance to be created > as helpers once there are similar use cases in other drivers(maybe, > there is/are already). > 3) Power management doesn't seem to be a problem, since each LCDIF has > it's own struct device which can be used to do runtime PM at some > drm_crtc_helper_funcs callbacks. > 4) Regarding the fake view of shared resources, atomic check can handle > that, so it doesn't seem to be a big problem, either. > >> >> While we do something similar on the GPU side and collect all GPU cores >> under a single DRM device, I'm not fully convinced that this was a good >> decision. It now comes back to bite us when the SoC topologies get a >> little more interesting and e.g. devices are behind different IOMMU >> streams. > > Right, SoC topologies may change, like the aforementioned muxes. > Generally speaking, I think one drm device is more flexible and > scalable than three. I agree with Lucas on one driver instance - one IP instance. Each IP instance is separate, so it should have separate driver instance bound to it.