[RFC] Revert "drm/panel-simple: drop use of data-mapping property"

Commit Message

Christoph Niedermaier Feb. 1, 2022, 11:07 a.m. UTC

Without the data-mapping devicetree property my display won't
work properly. It is flickering, because the bus flags won't
be assigned without a defined bus format by the imx parallel
display driver. There was a discussion about the removal [1]
and an agreement that a better solution is needed, but it is
missing so far. So what would be the better approach?

[1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1

This reverts commit d021d751c14752a0266865700f6f212fab40a18c.

Signed-off-by: Christoph Niedermaier <[e-mail]>
Cc: Marek Vasut <[e-mail]>
Cc: Sam Ravnborg <[e-mail]>
Cc: Laurent Pinchart <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Philipp Zabel <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Shawn Guo <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Pengutronix Kernel Team <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: NXP Linux Team <[e-mail]>
Cc: [e-mail]
To: [e-mail]
---
 drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Denys Drozdov Feb. 2, 2022, 3:42 p.m. UTC | #1

On 2/1/22 1:07 PM, Christoph Niedermaier wrote:
> Without the data-mapping devicetree property my display won't
> work properly. It is flickering, because the bus flags won't
> be assigned without a defined bus format by the imx parallel
> display driver. There was a discussion about the removal [1]
> and an agreement that a better solution is needed, but it is
> missing so far.

Obviously bus format in panel-dpi is broken, so this revert
looks necessary. The current state of panel-dpi isn't consistent
with changes introduced in commit 4a1d0dbc8332231d1d500d7a1d13c45457262a97

> So what would be the better approach?

Another approach is using panel instead of panel-dpi.

>
> [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
>
> This reverts commit d021d751c14752a0266865700f6f212fab40a18c.

But, also you need to revert 99f155d0776fb8838e326efce056aa08e25433d0

> Signed-off-by: Christoph Niedermaier <[e-mail]>

Tested-by: Denys Drozdov <[e-mail]>

Denys Drozdov Feb. 2, 2022, 3:54 p.m. UTC | #2

> On 2/1/22 1:07 PM, Christoph Niedermaier wrote:
>> Without the data-mapping devicetree property my display won't
>> work properly. It is flickering, because the bus flags won't
>> be assigned without a defined bus format by the imx parallel
>> display driver. There was a discussion about the removal [1]
>> and an agreement that a better solution is needed, but it is
>> missing so far.
> Obviously bus format in panel-dpi is broken, so this revert
> looks necessary. The current state of panel-dpi isn't consistent
> with changes introduced in commit 4a1d0dbc8332231d1d500d7a1d13c45457262a97
>> So what would be the better approach?
> Another approach is using panel instead of panel-dpi.
>> [1]https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
>>
>> This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
> But, also you need to revert 99f155d0776fb8838e326efce056aa08e25433d0
>> Signed-off-by: Christoph Niedermaier<[e-mail]>
> Tested-by: Denys Drozdov<[e-mail]>

Laurent Pinchart Feb. 2, 2022, 11:45 p.m. UTC | #3

Hi Christoph,

Thank you for the patch.

On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
> Without the data-mapping devicetree property my display won't
> work properly. It is flickering, because the bus flags won't
> be assigned without a defined bus format by the imx parallel
> display driver. There was a discussion about the removal [1]
> and an agreement that a better solution is needed, but it is
> missing so far. So what would be the better approach?
>
> [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
> 
> This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
> 
> Signed-off-by: Christoph Niedermaier <[e-mail]>
> Cc: Marek Vasut <[e-mail]>
> Cc: Sam Ravnborg <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Philipp Zabel <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Pengutronix Kernel Team <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: NXP Linux Team <[e-mail]>
> Cc: [e-mail]
> To: [e-mail]
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 3c08f9827acf..2c683d94a3f3 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
>  	struct panel_desc *desc;
>  	unsigned int bus_flags;
>  	struct videomode vm;
> +	const char *mapping;
>  	int ret;
>  
>  	np = dev->of_node;
> @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
>  	of_property_read_u32(np, "width-mm", &desc->size.width);
>  	of_property_read_u32(np, "height-mm", &desc->size.height);
>  
> +	of_property_read_string(np, "data-mapping", &mapping);
> +	if (!strcmp(mapping, "rgb24"))
> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	else if (!strcmp(mapping, "rgb565"))
> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> +	else if (!strcmp(mapping, "bgr666"))
> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> +	else if (!strcmp(mapping, "lvds666"))
> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;

You're right that there's an issue, but a revert isn't the right option.
The commit you're reverting never made it in a stable release, because
it was deemed to not be a good enough option.

First of all, any attempt to fix this should include an update to the DT
binding. Second, as this is about DPI panels, the LVDS option should be
dropped. Finally, I've shared some initial thoughts in [1], maybe you
can reply to that e-mail to continue the discussion there ?

https://lore.kernel.org/all/[e-mail]/

> +
>  	/* Extract bus_flags from display_timing */
>  	bus_flags = 0;
>  	vm.flags = timing->flags;

Alexander Stein Feb. 3, 2022, 8:01 a.m. UTC | #4

Hi Laurent,

Am Donnerstag, 3. Februar 2022, 00:45:59 CET schrieb Laurent Pinchart:
> [...]
> You're right that there's an issue, but a revert isn't the right option.
> The commit you're reverting never made it in a stable release, because
> it was deemed to not be a good enough option.
> 
> First of all, any attempt to fix this should include an update to the DT
> binding. Second, as this is about DPI panels, the LVDS option should be
> dropped. Finally, I've shared some initial thoughts in [1], maybe you
> can reply to that e-mail to continue the discussion there ?
> 
> https://lore.kernel.org/all/[e-mail]
> m/

At first I thought, this is a different issue than the one I currently have, 
but after reading this post, I think it's somewhat related.

> If a panel expects RGB888 and receives RGB666 with the two LSBs of each
> component hardwired to GND on the PCB, should DT report RGB888 or RGB666
> on the panel side ? I'm tempted by the former, and specifying the latter
> on the transmitting side.

My situation is the other way around. My panel (cdtech,s070swv29hg-dc44) has a 
MEDIA_BUS_FMT_RGB666_1X18 bus format (see panel-simple.c). Unfortunately for 
one mainboard the connection is like that:

i.MX -- Panel  (Blue and green is identical)
R7   --  R5
R6   --  R4
...
R2   --  R0
R1  dont care
R0  dont care

So the 8 bpc (imx) and 6 bps (panel) are MSB aligned. The 2 LSB are completely 
ignored.
The fast hacked fix is to use an additional panel description with bus format 
set to MEDIA_BUS_FMT_RGB888_1X24, keeping everything else the same. But that 
is cumbersome.
IMHO a straight forward solution is to use a, yet to be written, simple bridge 
which just converts the bus format transparently, assuming the electrical 
connection is actually correct.
This way the panel can set the native bus format, regardless of actual 
connections.
Christoph's problem should disappear as well if going that way, as the bus 
format is set for the <imx> -> <bridge> connection.
Nevertheless the panel bus format should be available in the end.

Regards,
Alexander

Christoph Niedermaier Feb. 8, 2022, 9:27 p.m. UTC | #5

From: Laurent Pinchart [mailto:[e-mail]]
Sent: Thursday, February 3, 2022 12:46 AM
> 
> Hi Christoph,
> 

Hi Laurent,

> On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
>> Without the data-mapping devicetree property my display won't
>> work properly. It is flickering, because the bus flags won't
>> be assigned without a defined bus format by the imx parallel
>> display driver. There was a discussion about the removal [1]
>> and an agreement that a better solution is needed, but it is
>> missing so far. So what would be the better approach?
>>
>> [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
>>
>> This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
>>
>> Signed-off-by: Christoph Niedermaier <[e-mail]>
>> Cc: Marek Vasut <[e-mail]>
>> Cc: Sam Ravnborg <[e-mail]>
>> Cc: Laurent Pinchart <[e-mail]>
>> Cc: Maxime Ripard <[e-mail]>
>> Cc: Philipp Zabel <[e-mail]>
>> Cc: David Airlie <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: Shawn Guo <[e-mail]>
>> Cc: Sascha Hauer <[e-mail]>
>> Cc: Pengutronix Kernel Team <[e-mail]>
>> Cc: Fabio Estevam <[e-mail]>
>> Cc: NXP Linux Team <[e-mail]>
>> Cc: [e-mail]
>> To: [e-mail]
>> ---
>>  drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index 3c08f9827acf..2c683d94a3f3 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
>>       struct panel_desc *desc;
>>       unsigned int bus_flags;
>>       struct videomode vm;
>> +     const char *mapping;
>>       int ret;
>>
>>       np = dev->of_node;
>> @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
>>       of_property_read_u32(np, "width-mm", &desc->size.width);
>>       of_property_read_u32(np, "height-mm", &desc->size.height);
>>
>> +     of_property_read_string(np, "data-mapping", &mapping);
>> +     if (!strcmp(mapping, "rgb24"))
>> +             desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>> +     else if (!strcmp(mapping, "rgb565"))
>> +             desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>> +     else if (!strcmp(mapping, "bgr666"))
>> +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>> +     else if (!strcmp(mapping, "lvds666"))
>> +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> 
> You're right that there's an issue, but a revert isn't the right option.
> The commit you're reverting never made it in a stable release, because
> it was deemed to not be a good enough option.
> 
> First of all, any attempt to fix this should include an update to the DT
> binding. Second, as this is about DPI panels, the LVDS option should be
> dropped. Finally, I've shared some initial thoughts in [1], maybe you
> can reply to that e-mail to continue the discussion there ?

According to your thoughts in [1] you mean that the bus format should be
build out of the devicetree properties bus-width and data-shift. It would
be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18,
but what about a bus like RGB565_1X16, where each color has different
bus width. Also the order of the colors should be defined to differ
between busses like RGB888_1X24 and GBR888_1X24.
Are there any ideas how can this be covered?

> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
>> +
>>       /* Extract bus_flags from display_timing */
>>       bus_flags = 0;
>>       vm.flags = timing->flags;
> 


Regards
Christoph

Marek Vasut Feb. 8, 2022, 11:52 p.m. UTC | #6

On 2/8/22 22:27, Christoph Niedermaier wrote:
> From: Laurent Pinchart [mailto:[e-mail]]
> Sent: Thursday, February 3, 2022 12:46 AM
>>
>> Hi Christoph,
>>
> 
> Hi Laurent,
> 
>> On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
>>> Without the data-mapping devicetree property my display won't
>>> work properly. It is flickering, because the bus flags won't
>>> be assigned without a defined bus format by the imx parallel
>>> display driver. There was a discussion about the removal [1]
>>> and an agreement that a better solution is needed, but it is
>>> missing so far. So what would be the better approach?
>>>
>>> [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
>>>
>>> This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
>>>
>>> Signed-off-by: Christoph Niedermaier <[e-mail]>
>>> Cc: Marek Vasut <[e-mail]>
>>> Cc: Sam Ravnborg <[e-mail]>
>>> Cc: Laurent Pinchart <[e-mail]>
>>> Cc: Maxime Ripard <[e-mail]>
>>> Cc: Philipp Zabel <[e-mail]>
>>> Cc: David Airlie <[e-mail]>
>>> Cc: Daniel Vetter <[e-mail]>
>>> Cc: Shawn Guo <[e-mail]>
>>> Cc: Sascha Hauer <[e-mail]>
>>> Cc: Pengutronix Kernel Team <[e-mail]>
>>> Cc: Fabio Estevam <[e-mail]>
>>> Cc: NXP Linux Team <[e-mail]>
>>> Cc: [e-mail]
>>> To: [e-mail]
>>> ---
>>>   drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>> index 3c08f9827acf..2c683d94a3f3 100644
>>> --- a/drivers/gpu/drm/panel/panel-simple.c
>>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>>> @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
>>>        struct panel_desc *desc;
>>>        unsigned int bus_flags;
>>>        struct videomode vm;
>>> +     const char *mapping;
>>>        int ret;
>>>
>>>        np = dev->of_node;
>>> @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
>>>        of_property_read_u32(np, "width-mm", &desc->size.width);
>>>        of_property_read_u32(np, "height-mm", &desc->size.height);
>>>
>>> +     of_property_read_string(np, "data-mapping", &mapping);
>>> +     if (!strcmp(mapping, "rgb24"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>> +     else if (!strcmp(mapping, "rgb565"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>> +     else if (!strcmp(mapping, "bgr666"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>> +     else if (!strcmp(mapping, "lvds666"))
>>> +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>
>> You're right that there's an issue, but a revert isn't the right option.
>> The commit you're reverting never made it in a stable release, because
>> it was deemed to not be a good enough option.
>>
>> First of all, any attempt to fix this should include an update to the DT
>> binding. Second, as this is about DPI panels, the LVDS option should be
>> dropped. Finally, I've shared some initial thoughts in [1], maybe you
>> can reply to that e-mail to continue the discussion there ?
> 
> According to your thoughts in [1] you mean that the bus format should be
> build out of the devicetree properties bus-width and data-shift. It would
> be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18,
> but what about a bus like RGB565_1X16, where each color has different
> bus width. Also the order of the colors should be defined to differ
> between busses like RGB888_1X24 and GBR888_1X24.
> Are there any ideas how can this be covered?

Maybe with props like these ?

channel-width -- width of each color channel
channel-shift -- shift of each color channel
channel-map -- mapping of each color channel

So for RGB888
channel-width = <8 8 8>;
channel-shift = <0 0 0>;
channel-map = "RGB"; // or something ?

For BGR565 panel connected to RGB24 scanout
channel-width = <5 6 5>;
channel-shift = <3 2 3>;
channel-map = "BGR"; // or something ?

For BGR565 panel connected to RGB565 scanout
channel-width = <5 6 5>;
channel-shift = <0 0 0>;
channel-map = "BGR"; // or something ?

Max Krummenacher Feb. 9, 2022, 1:14 p.m. UTC | #7

Hi

Am Mittwoch, den 09.02.2022, 00:52 +0100 schrieb Marek Vasut:
> On 2/8/22 22:27, Christoph Niedermaier wrote:
> > From: Laurent Pinchart [mailto:[e-mail]]
> > Sent: Thursday, February 3, 2022 12:46 AM
> > > Hi Christoph,
> > > 
> > 
> > Hi Laurent,
> > 
> > > On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
> > > > Without the data-mapping devicetree property my display won't
> > > > work properly. It is flickering, because the bus flags won't
> > > > be assigned without a defined bus format by the imx parallel
> > > > display driver. There was a discussion about the removal [1]
> > > > and an agreement that a better solution is needed, but it is
> > > > missing so far. So what would be the better approach?
> > > > 
> > > > [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
> > > > 
> > > > This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
> > > > 
> > > > Signed-off-by: Christoph Niedermaier <[e-mail]>
> > > > Cc: Marek Vasut <[e-mail]>
> > > > Cc: Sam Ravnborg <[e-mail]>
> > > > Cc: Laurent Pinchart <[e-mail]>
> > > > Cc: Maxime Ripard <[e-mail]>
> > > > Cc: Philipp Zabel <[e-mail]>
> > > > Cc: David Airlie <[e-mail]>
> > > > Cc: Daniel Vetter <[e-mail]>
> > > > Cc: Shawn Guo <[e-mail]>
> > > > Cc: Sascha Hauer <[e-mail]>
> > > > Cc: Pengutronix Kernel Team <[e-mail]>
> > > > Cc: Fabio Estevam <[e-mail]>
> > > > Cc: NXP Linux Team <[e-mail]>
> > > > Cc: [e-mail]
> > > > To: [e-mail]
> > > > ---
> > > >   drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
> > > >   1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > index 3c08f9827acf..2c683d94a3f3 100644
> > > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
> > > >        struct panel_desc *desc;
> > > >        unsigned int bus_flags;
> > > >        struct videomode vm;
> > > > +     const char *mapping;
> > > >        int ret;
> > > > 
> > > >        np = dev->of_node;
> > > > @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
> > > >        of_property_read_u32(np, "width-mm", &desc->size.width);
> > > >        of_property_read_u32(np, "height-mm", &desc->size.height);
> > > > 
> > > > +     of_property_read_string(np, "data-mapping", &mapping);
> > > > +     if (!strcmp(mapping, "rgb24"))
> > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > +     else if (!strcmp(mapping, "rgb565"))
> > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > +     else if (!strcmp(mapping, "bgr666"))
> > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > +     else if (!strcmp(mapping, "lvds666"))
> > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > 
> > > You're right that there's an issue, but a revert isn't the right option.
> > > The commit you're reverting never made it in a stable release, because
> > > it was deemed to not be a good enough option.
> > > 
> > > First of all, any attempt to fix this should include an update to the DT
> > > binding. Second, as this is about DPI panels, the LVDS option should be
> > > dropped. Finally, I've shared some initial thoughts in [1], maybe you
> > > can reply to that e-mail to continue the discussion there ?
> > 
> > According to your thoughts in [1] you mean that the bus format should be
> > build out of the devicetree properties bus-width and data-shift. It would
> > be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18,
> > but what about a bus like RGB565_1X16, where each color has different
> > bus width. Also the order of the colors should be defined to differ
> > between busses like RGB888_1X24 and GBR888_1X24.
> > Are there any ideas how can this be covered?
> 
> Maybe with props like these ?
> 
> channel-width -- width of each color channel
> channel-shift -- shift of each color channel
> channel-map -- mapping of each color channel
> 
> So for RGB888
> channel-width = <8 8 8>;
> channel-shift = <0 0 0>;
> channel-map = "RGB"; // or something ?
> 
> For BGR565 panel connected to RGB24 scanout
> channel-width = <5 6 5>;
> channel-shift = <3 2 3>;
> channel-map = "BGR"; // or something ?
> 
> For BGR565 panel connected to RGB565 scanout
> channel-width = <5 6 5>;
> channel-shift = <0 0 0>;
> channel-map = "BGR"; // or something ?
> 

To me this looks like it goes into the wrong direction.
The goal is to set bus_format to one of the
possible enum values.
Using such a generic approach it would be possible to request bus
formats for which no enum value is currently defined, i.e. the
BGR565 example does not have a matching bus_format in
media-bus-format.h.

What about keeping the solution which the revert would give.
Optionally with a changed property name s/data-mapping/bus-format/
and a changed list of allowed strings to better match between the
dt string and the enum name, i.e. s/"rgb24"/"RGB888_1X24"/.
This would allow to extend DT binding and
code to possible newly
required values like e.g. MEDIA_BUS_FMT_BGR888_1X24.

Alternatively one could use the bus-width and data-shift
properties and allow for (bus-width + data-shift) to be one
of 16, 18, 24 and assume RGB. I.e. one could only specify
MEDIA_BUS_FMT_RGB565_1X16, MEDIA_BUS_FMT_RGB666_1X18,
MEDIA_BUS_FMT_RGB888_1X24 respectively.

Max

> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Christoph Niedermaier Feb. 19, 2022, 9:37 a.m. UTC | #8

From: Max Krummenacher [mailto:[e-mail]]
Sent: Wednesday, February 9, 2022 2:14 PM
> 
> Hi
> 
> Am Mittwoch, den 09.02.2022, 00:52 +0100 schrieb Marek Vasut:
> > On 2/8/22 22:27, Christoph Niedermaier wrote:
> > > From: Laurent Pinchart [mailto:[e-mail]]
> > > Sent: Thursday, February 3, 2022 12:46 AM
> > > > Hi Christoph,
> > > >
> > >
> > > Hi Laurent,
> > >
> > > > On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
> > > > > Without the data-mapping devicetree property my display won't
> > > > > work properly. It is flickering, because the bus flags won't
> > > > > be assigned without a defined bus format by the imx parallel
> > > > > display driver. There was a discussion about the removal [1]
> > > > > and an agreement that a better solution is needed, but it is
> > > > > missing so far. So what would be the better approach?
> > > > >
> > > > > [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
> > > > >
> > > > > This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
> > > > >
> > > > > Signed-off-by: Christoph Niedermaier <[e-mail]>
> > > > > Cc: Marek Vasut <[e-mail]>
> > > > > Cc: Sam Ravnborg <[e-mail]>
> > > > > Cc: Laurent Pinchart <[e-mail]>
> > > > > Cc: Maxime Ripard <[e-mail]>
> > > > > Cc: Philipp Zabel <[e-mail]>
> > > > > Cc: David Airlie <[e-mail]>
> > > > > Cc: Daniel Vetter <[e-mail]>
> > > > > Cc: Shawn Guo <[e-mail]>
> > > > > Cc: Sascha Hauer <[e-mail]>
> > > > > Cc: Pengutronix Kernel Team <[e-mail]>
> > > > > Cc: Fabio Estevam <[e-mail]>
> > > > > Cc: NXP Linux Team <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > To: [e-mail]
> > > > > ---
> > > > >   drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
> > > > >   1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > > index 3c08f9827acf..2c683d94a3f3 100644
> > > > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > > > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
> > > > >        struct panel_desc *desc;
> > > > >        unsigned int bus_flags;
> > > > >        struct videomode vm;
> > > > > +     const char *mapping;
> > > > >        int ret;
> > > > >
> > > > >        np = dev->of_node;
> > > > > @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
> > > > >        of_property_read_u32(np, "width-mm", &desc->size.width);
> > > > >        of_property_read_u32(np, "height-mm", &desc->size.height);
> > > > >
> > > > > +     of_property_read_string(np, "data-mapping", &mapping);
> > > > > +     if (!strcmp(mapping, "rgb24"))
> > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > > +     else if (!strcmp(mapping, "rgb565"))
> > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > > +     else if (!strcmp(mapping, "bgr666"))
> > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > > +     else if (!strcmp(mapping, "lvds666"))
> > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > >
> > > > You're right that there's an issue, but a revert isn't the right option.
> > > > The commit you're reverting never made it in a stable release, because
> > > > it was deemed to not be a good enough option.
> > > >
> > > > First of all, any attempt to fix this should include an update to the DT
> > > > binding. Second, as this is about DPI panels, the LVDS option should be
> > > > dropped. Finally, I've shared some initial thoughts in [1], maybe you
> > > > can reply to that e-mail to continue the discussion there ?
> > >
> > > According to your thoughts in [1] you mean that the bus format should be
> > > build out of the devicetree properties bus-width and data-shift. It would
> > > be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18,
> > > but what about a bus like RGB565_1X16, where each color has different
> > > bus width. Also the order of the colors should be defined to differ
> > > between busses like RGB888_1X24 and GBR888_1X24.
> > > Are there any ideas how can this be covered?
> >
> > Maybe with props like these ?
> >
> > channel-width -- width of each color channel
> > channel-shift -- shift of each color channel
> > channel-map -- mapping of each color channel
> >
> > So for RGB888
> > channel-width = <8 8 8>;
> > channel-shift = <0 0 0>;
> > channel-map = "RGB"; // or something ?
> >
> > For BGR565 panel connected to RGB24 scanout
> > channel-width = <5 6 5>;
> > channel-shift = <3 2 3>;
> > channel-map = "BGR"; // or something ?
> >
> > For BGR565 panel connected to RGB565 scanout
> > channel-width = <5 6 5>;
> > channel-shift = <0 0 0>;
> > channel-map = "BGR"; // or something ?
> >
> 
> To me this looks like it goes into the wrong direction.
> The goal is to set bus_format to one of the
> possible enum values.
> Using such a generic approach it would be possible to request bus
> formats for which no enum value is currently defined, i.e. the
> BGR565 example does not have a matching bus_format in
> media-bus-format.h.
> 
> What about keeping the solution which the revert would give.
> Optionally with a changed property name s/data-mapping/bus-format/
> and a changed list of allowed strings to better match between the
> dt string and the enum name, i.e. s/"rgb24"/"RGB888_1X24"/.
> This would allow to extend DT binding and
> code to possible newly
> required values like e.g. MEDIA_BUS_FMT_BGR888_1X24.
> 
> Alternatively one could use the bus-width and data-shift
> properties and allow for (bus-width + data-shift) to be one
> of 16, 18, 24 and assume RGB. I.e. one could only specify
> MEDIA_BUS_FMT_RGB565_1X16, MEDIA_BUS_FMT_RGB666_1X18,
> MEDIA_BUS_FMT_RGB888_1X24 respectively.
> 
> Max
> 

Hi Laurent,

In which direction should it go to find an acceptable solution?

Regards
Christoph

Max Krummenacher Feb. 22, 2022, 8:51 a.m. UTC | #9

Am Samstag, den 19.02.2022, 09:37 +0000 schrieb Christoph Niedermaier:
> From: Max Krummenacher [mailto:[e-mail]]
> Sent: Wednesday, February 9, 2022 2:14 PM
> > Hi
> > 
> > Am Mittwoch, den 09.02.2022, 00:52 +0100 schrieb Marek Vasut:
> > > On 2/8/22 22:27, Christoph Niedermaier wrote:
> > > > From: Laurent Pinchart [mailto:[e-mail]]
> > > > Sent: Thursday, February 3, 2022 12:46 AM
> > > > > Hi Christoph,
> > > > > 
> > > > 
> > > > Hi Laurent,
> > > > 
> > > > > On Tue, Feb 01, 2022 at 12:07:17PM +0100, Christoph Niedermaier wrote:
> > > > > > Without the data-mapping devicetree property my display won't
> > > > > > work properly. It is flickering, because the bus flags won't
> > > > > > be assigned without a defined bus format by the imx parallel
> > > > > > display driver. There was a discussion about the removal [1]
> > > > > > and an agreement that a better solution is needed, but it is
> > > > > > missing so far. So what would be the better approach?
> > > > > > 
> > > > > > [1] https://patchwork.freedesktop.org/patch/357659/?series=74705&rev=1
> > > > > > 
> > > > > > This reverts commit d021d751c14752a0266865700f6f212fab40a18c.
> > > > > > 
> > > > > > Signed-off-by: Christoph Niedermaier <[e-mail]>
> > > > > > Cc: Marek Vasut <[e-mail]>
> > > > > > Cc: Sam Ravnborg <[e-mail]>
> > > > > > Cc: Laurent Pinchart <[e-mail]>
> > > > > > Cc: Maxime Ripard <[e-mail]>
> > > > > > Cc: Philipp Zabel <[e-mail]>
> > > > > > Cc: David Airlie <[e-mail]>
> > > > > > Cc: Daniel Vetter <[e-mail]>
> > > > > > Cc: Shawn Guo <[e-mail]>
> > > > > > Cc: Sascha Hauer <[e-mail]>
> > > > > > Cc: Pengutronix Kernel Team <[e-mail]>
> > > > > > Cc: Fabio Estevam <[e-mail]>
> > > > > > Cc: NXP Linux Team <[e-mail]>
> > > > > > Cc: [e-mail]
> > > > > > To: [e-mail]
> > > > > > ---
> > > > > >   drivers/gpu/drm/panel/panel-simple.c | 11 +++++++++++
> > > > > >   1 file changed, 11 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > > > index 3c08f9827acf..2c683d94a3f3 100644
> > > > > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > > > > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev,
> > > > > >        struct panel_desc *desc;
> > > > > >        unsigned int bus_flags;
> > > > > >        struct videomode vm;
> > > > > > +     const char *mapping;
> > > > > >        int ret;
> > > > > > 
> > > > > >        np = dev->of_node;
> > > > > > @@ -477,6 +478,16 @@ static int panel_dpi_probe(struct device *dev,
> > > > > >        of_property_read_u32(np, "width-mm", &desc->size.width);
> > > > > >        of_property_read_u32(np, "height-mm", &desc->size.height);
> > > > > > 
> > > > > > +     of_property_read_string(np, "data-mapping", &mapping);
> > > > > > +     if (!strcmp(mapping, "rgb24"))
> > > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > > > +     else if (!strcmp(mapping, "rgb565"))
> > > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > > > +     else if (!strcmp(mapping, "bgr666"))
> > > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > > > +     else if (!strcmp(mapping, "lvds666"))
> > > > > > +             desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > > > 
> > > > > You're right that there's an issue, but a revert isn't the right option.
> > > > > The commit you're reverting never made it in a stable release, because
> > > > > it was deemed to not be a good enough option.
> > > > > 
> > > > > First of all, any attempt to fix this should include an update to the DT
> > > > > binding. Second, as this is about DPI panels, the LVDS option should be
> > > > > dropped. Finally, I've shared some initial thoughts in [1], maybe you
> > > > > can reply to that e-mail to continue the discussion there ?
> > > > 
> > > > According to your thoughts in [1] you mean that the bus format should be
> > > > build out of the devicetree properties bus-width and data-shift. It would
> > > > be possible for evenly structured busses like RGB888_1X24 and RGB666_1X18,
> > > > but what about a bus like RGB565_1X16, where each color has different
> > > > bus width. Also the order of the colors should be defined to differ
> > > > between busses like RGB888_1X24 and GBR888_1X24.
> > > > Are there any ideas how can this be covered?
> > > 
> > > Maybe with props like these ?
> > > 
> > > channel-width -- width of each color channel
> > > channel-shift -- shift of each color channel
> > > channel-map -- mapping of each color channel
> > > 
> > > So for RGB888
> > > channel-width = <8 8 8>;
> > > channel-shift = <0 0 0>;
> > > channel-map = "RGB"; // or something ?
> > > 
> > > For BGR565 panel connected to RGB24 scanout
> > > channel-width = <5 6 5>;
> > > channel-shift = <3 2 3>;
> > > channel-map = "BGR"; // or something ?
> > > 
> > > For BGR565 panel connected to RGB565 scanout
> > > channel-width = <5 6 5>;
> > > channel-shift = <0 0 0>;
> > > channel-map = "BGR"; // or something ?
> > > 
> > 
> > To me this looks like it goes into the wrong direction.
> > The goal is to set bus_format to one of the
> > possible enum values.
> > Using such a generic approach it would be possible to request bus
> > formats for which no enum value is currently defined, i.e. the
> > BGR565 example does not have a matching bus_format in
> > media-bus-format.h.
> > 
> > What about keeping the solution which the revert would give.
> > Optionally with a changed property name s/data-mapping/bus-format/
> > and a changed list of allowed strings to better match between the
> > dt string and the enum name, i.e. s/"rgb24"/"RGB888_1X24"/.
> > This would allow to extend DT binding and
> > code to possible newly
> > required values like e.g. MEDIA_BUS_FMT_BGR888_1X24.
> > 
> > Alternatively one could use the bus-width and data-shift
> > properties and allow for (bus-width + data-shift) to be one
> > of 16, 18, 24 and assume RGB. I.e. one could only specify
> > MEDIA_BUS_FMT_RGB565_1X16, MEDIA_BUS_FMT_RGB666_1X18,
> > MEDIA_BUS_FMT_RGB888_1X24 respectively.
> > 
> > Max
> > 
> 
> Hi Laurent,
> 
> In which direction should it go to find an acceptable solution?

Hi

I sent a proposal as RFC to show how this could look in the implementation.
https://lore.kernel.org/all/[e-mail]/

Max
> 
> Regards
> Christoph

Patch

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 3c08f9827acf..2c683d94a3f3 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -453,6 +453,7 @@  static int panel_dpi_probe(struct device *dev,
 	struct panel_desc *desc;
 	unsigned int bus_flags;
 	struct videomode vm;
+	const char *mapping;
 	int ret;
 
 	np = dev->of_node;
@@ -477,6 +478,16 @@  static int panel_dpi_probe(struct device *dev,
 	of_property_read_u32(np, "width-mm", &desc->size.width);
 	of_property_read_u32(np, "height-mm", &desc->size.height);
 
+	of_property_read_string(np, "data-mapping", &mapping);
+	if (!strcmp(mapping, "rgb24"))
+		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	else if (!strcmp(mapping, "rgb565"))
+		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
+	else if (!strcmp(mapping, "bgr666"))
+		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+	else if (!strcmp(mapping, "lvds666"))
+		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+
 	/* Extract bus_flags from display_timing */
 	bus_flags = 0;
 	vm.flags = timing->flags;