From: Dave Stevenson <dave.stevenson@raspberrypi.com> To: Maxime Ripard <maxime@cerno.tech> Cc: Marek Vasut <marex@denx.de>, Christoph Niedermaier <cniedermaier@dh-electronics.com>, Max Krummenacher <max.krummenacher@toradex.com>, Max Krummenacher <max.oss.09@gmail.com>, David Airlie <airlied@linux.ie>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, DRI Development <dri-devel@lists.freedesktop.org>, DenysDrozdov <denys.drozdov@toradex.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Pengutronix Kernel Team <kernel@pengutronix.de>, Sam Ravnborg <sam@ravnborg.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, NXP Linux Team <linux-imx@nxp.com> Subject: Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format Date: Fri, 18 Mar 2022 17:05:11 +0000 [thread overview] Message-ID: <CAPY8ntDgWwXyWXDWVouzhdC2wsyjbRgfrvWGU=MRG_2sAquHyQ@mail.gmail.com> (raw) In-Reply-To: <20220318163549.5a5v3lex4btnnvgb@houat> Hi Maxime On Fri, 18 Mar 2022 at 16:35, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Mar 07, 2022 at 04:26:56PM +0100, Max Krummenacher wrote: > > On Wed, Mar 2, 2022 at 5:22 PM Marek Vasut <marex@denx.de> wrote: > > > > > > On 3/2/22 15:21, Maxime Ripard wrote: > > > > Hi, > > > > > > Hi, > > > > > > > Please try to avoid top posting > > Sorry. > > > > > > > > > > On Wed, Feb 23, 2022 at 04:25:19PM +0100, Max Krummenacher wrote: > > > >> The goal here is to set the element bus_format in the struct > > > >> panel_desc. This is an enum with the possible values defined in > > > >> include/uapi/linux/media-bus-format.h. > > > >> > > > >> The enum values are not constructed in a way that you could calculate > > > >> the value from color channel width/shift/mapping/whatever. You rather > > > >> would have to check if the combination of color channel > > > >> width/shift/mapping/whatever maps to an existing value and otherwise > > > >> EINVAL out. > > > >> > > > >> I don't see the value in having yet another way of how this > > > >> information can be specified and then having to write a more > > > >> complicated parser which maps the dt data to bus_format. > > > > > > > > Generally speaking, sending an RFC without explicitly stating what you > > > > want a comment on isn't very efficient. > > > > > > Isn't that what RFC stands for -- Request For Comment ? > > > > I hoped that the link to the original discussion was enough. > > > > panel-simple used to have a finite number of hardcoded panels selected > > by their compatible. > > The following patchsets added a compatible 'panel-dpi' which should > > allow to specify the panel in the device tree with timing etc. > > https://patchwork.kernel.org/project/dri-devel/patch/20200216181513.28109-6-sam@ravnborg.org/ > > In the same release cycle part of it got reverted: > > https://patchwork.kernel.org/project/dri-devel/patch/20200314153047.2486-3-sam@ravnborg.org/ > > With this it is no longer possible to set bus_format. > > > > The explanation what makes the use of a property "data-mapping" not a > > suitable way in that revert > > is a bit vague. > > Indeed, but I can only guess. BGR666 in itself doesn't mean much for > example. Chances are the DPI interface will use a 24 bit bus, so where > is the padding? > > I think that's what Sam and Laurent were talking about: there wasn't > enough information encoded in that property to properly describe the > format, hence the revert. MEDIA_BUS_FMT_RGB666_1X18 defines an 18bit bus, therefore there is no padding. "bgr666" was selecting that media bus code (I won't ask about the rgb/bgr swap). If there is padding on a 24 bit bus, then you'd use (for example) MEDIA_BUS_FMT_RGB666_1X24_CPADHI to denote that the top 2 bits of each colour are the padding. Define and use a PADLO variant if the padding is the low bits. The string matching would need to be extended to have some string to select those codes ("lvds666" is a weird choice from the original patch). Taking those media bus codes and handling them appropriately is already done in vc4_dpi [1], and the vendor tree has gained BGR666_1X18 and BGR666_1X24_CPADHI [2] as they aren't defined in mainline. Now this does potentially balloon out the number of MEDIA_BUS_FMT_xxx defines needed, but that's the downside of having defines for all formats. (I will admit to having a similar change in the Pi vendor tree that allows the media bus code to be selected explicitly by hex value). Dave [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_dpi.c#L154 [2] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/include/uapi/linux/media-bus-format.h#L49 > > The RFC revert of the revert > > https://patchwork.kernel.org/project/dri-devel/patch/20220201110717.3585-1-cniedermaier@dh-electronics.com/ > > tried to get feedback what would be a way forward. This RFC tries the > > same by giving a possible solution should > > the property name and/or the a bit short strings of the original be > > the reason why it is not suitable. > > > > So the requested comments would be about what was not good enough with > > 'data-mapping' and what would be a way forward. > > > > Especially since in my limited view it is not clear why in panel-lvds > > 'data-mapping' is used to state how the bits representing colour are > > mapped to the 21 or 28 possible bit position in the LVDS lanes vs. > > here where we want to say how the bits representing colour are mapped > > to the 16/18/24 lines of the parallel interface would need a different > > binding pattern. > > There's only a few data format in LVDS, so it's ok. A DPI interface is > much more free-form, so you need to be a bit more accurate than what is > done for LVDS. > > Maxime
WARNING: multiple messages have this Message-ID (diff)
From: Dave Stevenson <dave.stevenson@raspberrypi.com> To: Maxime Ripard <maxime@cerno.tech> Cc: Max Krummenacher <max.oss.09@gmail.com>, Marek Vasut <marex@denx.de>, Christoph Niedermaier <cniedermaier@dh-electronics.com>, Max Krummenacher <max.krummenacher@toradex.com>, Pengutronix Kernel Team <kernel@pengutronix.de>, David Airlie <airlied@linux.ie>, Sam Ravnborg <sam@ravnborg.org>, Sascha Hauer <s.hauer@pengutronix.de>, DRI Development <dri-devel@lists.freedesktop.org>, DenysDrozdov <denys.drozdov@toradex.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Shawn Guo <shawnguo@kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, NXP Linux Team <linux-imx@nxp.com> Subject: Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format Date: Fri, 18 Mar 2022 17:05:11 +0000 [thread overview] Message-ID: <CAPY8ntDgWwXyWXDWVouzhdC2wsyjbRgfrvWGU=MRG_2sAquHyQ@mail.gmail.com> (raw) In-Reply-To: <20220318163549.5a5v3lex4btnnvgb@houat> Hi Maxime On Fri, 18 Mar 2022 at 16:35, Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Mar 07, 2022 at 04:26:56PM +0100, Max Krummenacher wrote: > > On Wed, Mar 2, 2022 at 5:22 PM Marek Vasut <marex@denx.de> wrote: > > > > > > On 3/2/22 15:21, Maxime Ripard wrote: > > > > Hi, > > > > > > Hi, > > > > > > > Please try to avoid top posting > > Sorry. > > > > > > > > > > On Wed, Feb 23, 2022 at 04:25:19PM +0100, Max Krummenacher wrote: > > > >> The goal here is to set the element bus_format in the struct > > > >> panel_desc. This is an enum with the possible values defined in > > > >> include/uapi/linux/media-bus-format.h. > > > >> > > > >> The enum values are not constructed in a way that you could calculate > > > >> the value from color channel width/shift/mapping/whatever. You rather > > > >> would have to check if the combination of color channel > > > >> width/shift/mapping/whatever maps to an existing value and otherwise > > > >> EINVAL out. > > > >> > > > >> I don't see the value in having yet another way of how this > > > >> information can be specified and then having to write a more > > > >> complicated parser which maps the dt data to bus_format. > > > > > > > > Generally speaking, sending an RFC without explicitly stating what you > > > > want a comment on isn't very efficient. > > > > > > Isn't that what RFC stands for -- Request For Comment ? > > > > I hoped that the link to the original discussion was enough. > > > > panel-simple used to have a finite number of hardcoded panels selected > > by their compatible. > > The following patchsets added a compatible 'panel-dpi' which should > > allow to specify the panel in the device tree with timing etc. > > https://patchwork.kernel.org/project/dri-devel/patch/20200216181513.28109-6-sam@ravnborg.org/ > > In the same release cycle part of it got reverted: > > https://patchwork.kernel.org/project/dri-devel/patch/20200314153047.2486-3-sam@ravnborg.org/ > > With this it is no longer possible to set bus_format. > > > > The explanation what makes the use of a property "data-mapping" not a > > suitable way in that revert > > is a bit vague. > > Indeed, but I can only guess. BGR666 in itself doesn't mean much for > example. Chances are the DPI interface will use a 24 bit bus, so where > is the padding? > > I think that's what Sam and Laurent were talking about: there wasn't > enough information encoded in that property to properly describe the > format, hence the revert. MEDIA_BUS_FMT_RGB666_1X18 defines an 18bit bus, therefore there is no padding. "bgr666" was selecting that media bus code (I won't ask about the rgb/bgr swap). If there is padding on a 24 bit bus, then you'd use (for example) MEDIA_BUS_FMT_RGB666_1X24_CPADHI to denote that the top 2 bits of each colour are the padding. Define and use a PADLO variant if the padding is the low bits. The string matching would need to be extended to have some string to select those codes ("lvds666" is a weird choice from the original patch). Taking those media bus codes and handling them appropriately is already done in vc4_dpi [1], and the vendor tree has gained BGR666_1X18 and BGR666_1X24_CPADHI [2] as they aren't defined in mainline. Now this does potentially balloon out the number of MEDIA_BUS_FMT_xxx defines needed, but that's the downside of having defines for all formats. (I will admit to having a similar change in the Pi vendor tree that allows the media bus code to be selected explicitly by hex value). Dave [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_dpi.c#L154 [2] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/include/uapi/linux/media-bus-format.h#L49 > > The RFC revert of the revert > > https://patchwork.kernel.org/project/dri-devel/patch/20220201110717.3585-1-cniedermaier@dh-electronics.com/ > > tried to get feedback what would be a way forward. This RFC tries the > > same by giving a possible solution should > > the property name and/or the a bit short strings of the original be > > the reason why it is not suitable. > > > > So the requested comments would be about what was not good enough with > > 'data-mapping' and what would be a way forward. > > > > Especially since in my limited view it is not clear why in panel-lvds > > 'data-mapping' is used to state how the bits representing colour are > > mapped to the 21 or 28 possible bit position in the LVDS lanes vs. > > here where we want to say how the bits representing colour are mapped > > to the 16/18/24 lines of the parallel interface would need a different > > binding pattern. > > There's only a few data format in LVDS, so it's ok. A DPI interface is > much more free-form, so you need to be a bit more accurate than what is > done for LVDS. > > Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-18 17:05 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-22 8:47 [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format Max Krummenacher 2022-02-22 8:47 ` Max Krummenacher 2022-02-23 13:41 ` Maxime Ripard 2022-02-23 13:41 ` Maxime Ripard 2022-02-23 13:45 ` Marek Vasut 2022-02-23 13:45 ` Marek Vasut 2022-02-23 13:47 ` Maxime Ripard 2022-02-23 13:47 ` Maxime Ripard 2022-02-23 14:09 ` Marek Vasut 2022-02-23 14:09 ` Marek Vasut 2022-02-23 14:37 ` Maxime Ripard 2022-02-23 14:37 ` Maxime Ripard 2022-02-23 14:38 ` Marek Vasut 2022-02-23 14:38 ` Marek Vasut 2022-02-23 16:39 ` Maxime Ripard 2022-02-23 16:39 ` Maxime Ripard 2022-02-23 16:57 ` Marek Vasut 2022-02-23 16:57 ` Marek Vasut 2022-02-23 15:25 ` Max Krummenacher 2022-02-23 15:25 ` Max Krummenacher 2022-03-02 14:21 ` Maxime Ripard 2022-03-02 14:21 ` Maxime Ripard 2022-03-02 16:22 ` Marek Vasut 2022-03-02 16:22 ` Marek Vasut 2022-03-07 15:26 ` Max Krummenacher 2022-03-07 15:26 ` Max Krummenacher 2022-03-18 16:35 ` Maxime Ripard 2022-03-18 16:35 ` Maxime Ripard 2022-03-18 17:05 ` Dave Stevenson [this message] 2022-03-18 17:05 ` Dave Stevenson 2022-03-18 17:16 ` Maxime Ripard 2022-03-18 17:16 ` Maxime Ripard 2022-03-18 17:53 ` Dave Stevenson 2022-03-18 17:53 ` Dave Stevenson 2022-03-23 8:42 ` Max Krummenacher 2022-03-23 8:42 ` Max Krummenacher 2022-03-23 15:58 ` Maxime Ripard 2022-03-23 15:58 ` Maxime Ripard 2022-03-23 20:06 ` Max Krummenacher 2022-03-23 20:06 ` Max Krummenacher 2022-03-24 8:15 ` Francesco Dolcini 2022-03-24 8:15 ` Francesco Dolcini 2022-04-08 18:01 ` Laurent Pinchart 2022-04-08 18:01 ` Laurent Pinchart 2022-04-08 18:15 ` Laurent Pinchart 2022-04-08 18:15 ` Laurent Pinchart 2022-04-19 11:50 ` Max Krummenacher 2022-04-19 11:50 ` Max Krummenacher
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAPY8ntDgWwXyWXDWVouzhdC2wsyjbRgfrvWGU=MRG_2sAquHyQ@mail.gmail.com' \ --to=dave.stevenson@raspberrypi.com \ --cc=airlied@linux.ie \ --cc=cniedermaier@dh-electronics.com \ --cc=denys.drozdov@toradex.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=kernel@pengutronix.de \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=marex@denx.de \ --cc=max.krummenacher@toradex.com \ --cc=max.oss.09@gmail.com \ --cc=maxime@cerno.tech \ --cc=s.hauer@pengutronix.de \ --cc=sam@ravnborg.org \ --cc=shawnguo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.