[RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Use the new property bus-format to set the enum bus_format and bpc.
Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>

---

 drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/

Max

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index c5f133667a2d..5c07260de71c 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 *format = "";
 	int ret;
 
 	np = dev->of_node;
@@ -477,6 +478,37 @@ 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, "bus-format", &format);
+	if (!strcmp(format, "BGR888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
+	} else if (!strcmp(format, "GBR888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
+	} else if (!strcmp(format, "RBG888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
+	} else if (!strcmp(format, "RGB444_1X12")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
+	} else if (!strcmp(format, "RGB565_1X16")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
+	} else if (!strcmp(format, "RGB666_1X18")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+	} else if (!strcmp(format, "RGB888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	} else {
+		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
+			np);
+		return -EINVAL;
+	}
+
 	/* Extract bus_flags from display_timing */
 	bus_flags = 0;
 	vm.flags = timing->flags;
-- 
2.20.1

[RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Use the new property bus-format to set the enum bus_format and bpc.
Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>

---

 drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/

Max

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index c5f133667a2d..5c07260de71c 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 *format = "";
 	int ret;
 
 	np = dev->of_node;
@@ -477,6 +478,37 @@ 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, "bus-format", &format);
+	if (!strcmp(format, "BGR888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
+	} else if (!strcmp(format, "GBR888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
+	} else if (!strcmp(format, "RBG888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
+	} else if (!strcmp(format, "RGB444_1X12")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
+	} else if (!strcmp(format, "RGB565_1X16")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
+	} else if (!strcmp(format, "RGB666_1X18")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
+	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
+		desc->bpc = 6;
+		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+	} else if (!strcmp(format, "RGB888_1X24")) {
+		desc->bpc = 8;
+		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	} else {
+		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
+			np);
+		return -EINVAL;
+	}
+
 	/* Extract bus_flags from display_timing */
 	bus_flags = 0;
 	vm.flags = timing->flags;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

Hi,

On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> Use the new property bus-format to set the enum bus_format and bpc.
> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> 
> ---
> 
>  drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> 
> Max
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index c5f133667a2d..5c07260de71c 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 *format = "";
>  	int ret;
>  
>  	np = dev->of_node;
> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> +	if (!strcmp(format, "BGR888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> +	} else if (!strcmp(format, "GBR888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> +	} else if (!strcmp(format, "RBG888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> +	} else if (!strcmp(format, "RGB444_1X12")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> +	} else if (!strcmp(format, "RGB565_1X16")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> +	} else if (!strcmp(format, "RGB666_1X18")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> +	} else if (!strcmp(format, "RGB888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	} else {
> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> +			np);
> +		return -EINVAL;
> +	}
> +

It doesn't seem right, really. We can't the bus format / bpc be inferred
from the compatible? I'd expect two panels that don't have the same bus
format to not be claimed as compatible.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2587 bytes --]

Hi,

On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> Use the new property bus-format to set the enum bus_format and bpc.
> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> 
> ---
> 
>  drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> 
> Max
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index c5f133667a2d..5c07260de71c 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 *format = "";
>  	int ret;
>  
>  	np = dev->of_node;
> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> +	if (!strcmp(format, "BGR888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> +	} else if (!strcmp(format, "GBR888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> +	} else if (!strcmp(format, "RBG888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> +	} else if (!strcmp(format, "RGB444_1X12")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> +	} else if (!strcmp(format, "RGB565_1X16")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> +	} else if (!strcmp(format, "RGB666_1X18")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> +		desc->bpc = 6;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> +	} else if (!strcmp(format, "RGB888_1X24")) {
> +		desc->bpc = 8;
> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +	} else {
> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> +			np);
> +		return -EINVAL;
> +	}
> +

It doesn't seem right, really. We can't the bus format / bpc be inferred
from the compatible? I'd expect two panels that don't have the same bus
format to not be claimed as compatible.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 14:41, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>> Use the new property bus-format to set the enum bus_format and bpc.
>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>
>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>
>> ---
>>
>>   drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>
>> Max
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>   	int ret;
>>   
>>   	np = dev->of_node;
>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>> +	if (!strcmp(format, "BGR888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>> +	} else {
>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>> +			np);
>> +		return -EINVAL;
>> +	}
>> +
> 
> It doesn't seem right, really. We can't the bus format / bpc be inferred
> from the compatible? I'd expect two panels that don't have the same bus
> format to not be claimed as compatible.

Which compatible ?

Note that this is for panel-dpi compatible, i.e. the panel which has 
timings specified in DT (and needs bus format specified there too).

I agree this doesn't look right however, some more generic color channel 
width/shift/mapping might be better.

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 14:41, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>> Use the new property bus-format to set the enum bus_format and bpc.
>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>
>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>
>> ---
>>
>>   drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>
>> Max
>>
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>   	int ret;
>>   
>>   	np = dev->of_node;
>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>> +	if (!strcmp(format, "BGR888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>> +		desc->bpc = 6;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>> +		desc->bpc = 8;
>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>> +	} else {
>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>> +			np);
>> +		return -EINVAL;
>> +	}
>> +
> 
> It doesn't seem right, really. We can't the bus format / bpc be inferred
> from the compatible? I'd expect two panels that don't have the same bus
> format to not be claimed as compatible.

Which compatible ?

Note that this is for panel-dpi compatible, i.e. the panel which has 
timings specified in DT (and needs bus format specified there too).

I agree this doesn't look right however, some more generic color channel 
width/shift/mapping might be better.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3272 bytes --]

On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
> On 2/23/22 14:41, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> > > Use the new property bus-format to set the enum bus_format and bpc.
> > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> > > 
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > 
> > > ---
> > > 
> > >   drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> > >   1 file changed, 32 insertions(+)
> > > 
> > > Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> > > 
> > > Max
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index c5f133667a2d..5c07260de71c 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 *format = "";
> > >   	int ret;
> > >   	np = dev->of_node;
> > > @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> > > +	if (!strcmp(format, "BGR888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > > +	} else if (!strcmp(format, "GBR888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > > +	} else if (!strcmp(format, "RBG888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > > +	} else if (!strcmp(format, "RGB444_1X12")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> > > +	} else if (!strcmp(format, "RGB565_1X16")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > +	} else if (!strcmp(format, "RGB666_1X18")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > +	} else if (!strcmp(format, "RGB888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > +	} else {
> > > +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> > > +			np);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > from the compatible? I'd expect two panels that don't have the same bus
> > format to not be claimed as compatible.
> 
> Which compatible ?
> 
> Note that this is for panel-dpi compatible, i.e. the panel which has timings
> specified in DT (and needs bus format specified there too).

panel-dpi is supposed to have two compatibles, the panel-specific one
and panel-dpi. This would obviously be tied to the panel-specific one.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3272 bytes --]

On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
> On 2/23/22 14:41, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> > > Use the new property bus-format to set the enum bus_format and bpc.
> > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> > > 
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > 
> > > ---
> > > 
> > >   drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> > >   1 file changed, 32 insertions(+)
> > > 
> > > Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> > > 
> > > Max
> > > 
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index c5f133667a2d..5c07260de71c 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 *format = "";
> > >   	int ret;
> > >   	np = dev->of_node;
> > > @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> > > +	if (!strcmp(format, "BGR888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > > +	} else if (!strcmp(format, "GBR888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > > +	} else if (!strcmp(format, "RBG888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > > +	} else if (!strcmp(format, "RGB444_1X12")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> > > +	} else if (!strcmp(format, "RGB565_1X16")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > +	} else if (!strcmp(format, "RGB666_1X18")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> > > +		desc->bpc = 6;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > +	} else if (!strcmp(format, "RGB888_1X24")) {
> > > +		desc->bpc = 8;
> > > +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > +	} else {
> > > +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> > > +			np);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > from the compatible? I'd expect two panels that don't have the same bus
> > format to not be claimed as compatible.
> 
> Which compatible ?
> 
> Note that this is for panel-dpi compatible, i.e. the panel which has timings
> specified in DT (and needs bus format specified there too).

panel-dpi is supposed to have two compatibles, the panel-specific one
and panel-dpi. This would obviously be tied to the panel-specific one.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 14:47, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>> On 2/23/22 14:41, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>
>>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>>>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>>>
>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>
>>>> Max
>>>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>    	int ret;
>>>>    	np = dev->of_node;
>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>> +	} else {
>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>> +			np);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>
>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>> from the compatible? I'd expect two panels that don't have the same bus
>>> format to not be claimed as compatible.
>>
>> Which compatible ?
>>
>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>> specified in DT (and needs bus format specified there too).
> 
> panel-dpi is supposed to have two compatibles, the panel-specific one
> and panel-dpi. This would obviously be tied to the panel-specific one.

This whole discussion is about the one which only has 'panel-dpi' 
compatible and describes the panel in DT completely. The specific 
compatible is not present in DT when this patch is needed.

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 14:47, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>> On 2/23/22 14:41, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>
>>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>>>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>>>
>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>
>>>> Max
>>>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>    	int ret;
>>>>    	np = dev->of_node;
>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>> +		desc->bpc = 6;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>> +		desc->bpc = 8;
>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>> +	} else {
>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>> +			np);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>
>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>> from the compatible? I'd expect two panels that don't have the same bus
>>> format to not be claimed as compatible.
>>
>> Which compatible ?
>>
>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>> specified in DT (and needs bus format specified there too).
> 
> panel-dpi is supposed to have two compatibles, the panel-specific one
> and panel-dpi. This would obviously be tied to the panel-specific one.

This whole discussion is about the one which only has 'panel-dpi' 
compatible and describes the panel in DT completely. The specific 
compatible is not present in DT when this patch is needed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4187 bytes --]

On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
> On 2/23/22 14:47, Maxime Ripard wrote:
> > On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
> > > On 2/23/22 14:41, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> > > > > Use the new property bus-format to set the enum bus_format and bpc.
> > > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> > > > > 
> > > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > >    drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> > > > >    1 file changed, 32 insertions(+)
> > > > > 
> > > > > Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> > > > > 
> > > > > Max
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > > index c5f133667a2d..5c07260de71c 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 *format = "";
> > > > >    	int ret;
> > > > >    	np = dev->of_node;
> > > > > @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> > > > > +	if (!strcmp(format, "BGR888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > > > > +	} else if (!strcmp(format, "GBR888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > > > > +	} else if (!strcmp(format, "RBG888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > > > > +	} else if (!strcmp(format, "RGB444_1X12")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> > > > > +	} else if (!strcmp(format, "RGB565_1X16")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > > +	} else if (!strcmp(format, "RGB666_1X18")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > > +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > > > +	} else if (!strcmp(format, "RGB888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > > +	} else {
> > > > > +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> > > > > +			np);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > 
> > > > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > > > from the compatible? I'd expect two panels that don't have the same bus
> > > > format to not be claimed as compatible.
> > > 
> > > Which compatible ?
> > > 
> > > Note that this is for panel-dpi compatible, i.e. the panel which has timings
> > > specified in DT (and needs bus format specified there too).
> > 
> > panel-dpi is supposed to have two compatibles, the panel-specific one
> > and panel-dpi. This would obviously be tied to the panel-specific one.
> 
> This whole discussion is about the one which only has 'panel-dpi' compatible
> and describes the panel in DT completely. The specific compatible is not
> present in DT when this patch is needed.

From the panel-dpi DT binding:

properties:
  compatible:
    description:
      Shall contain a panel specific compatible and "panel-dpi"
      in that order.
    items:
      - {}
      - const: panel-dpi

The panel-specific compatible is mandatory, whether you like it or not.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4187 bytes --]

On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
> On 2/23/22 14:47, Maxime Ripard wrote:
> > On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
> > > On 2/23/22 14:41, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> > > > > Use the new property bus-format to set the enum bus_format and bpc.
> > > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> > > > > 
> > > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > >    drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> > > > >    1 file changed, 32 insertions(+)
> > > > > 
> > > > > Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> > > > > 
> > > > > Max
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > > index c5f133667a2d..5c07260de71c 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 *format = "";
> > > > >    	int ret;
> > > > >    	np = dev->of_node;
> > > > > @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> > > > > +	if (!strcmp(format, "BGR888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > > > > +	} else if (!strcmp(format, "GBR888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > > > > +	} else if (!strcmp(format, "RBG888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > > > > +	} else if (!strcmp(format, "RGB444_1X12")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> > > > > +	} else if (!strcmp(format, "RGB565_1X16")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > > +	} else if (!strcmp(format, "RGB666_1X18")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > > +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> > > > > +		desc->bpc = 6;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > > > +	} else if (!strcmp(format, "RGB888_1X24")) {
> > > > > +		desc->bpc = 8;
> > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > > +	} else {
> > > > > +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> > > > > +			np);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > 
> > > > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > > > from the compatible? I'd expect two panels that don't have the same bus
> > > > format to not be claimed as compatible.
> > > 
> > > Which compatible ?
> > > 
> > > Note that this is for panel-dpi compatible, i.e. the panel which has timings
> > > specified in DT (and needs bus format specified there too).
> > 
> > panel-dpi is supposed to have two compatibles, the panel-specific one
> > and panel-dpi. This would obviously be tied to the panel-specific one.
> 
> This whole discussion is about the one which only has 'panel-dpi' compatible
> and describes the panel in DT completely. The specific compatible is not
> present in DT when this patch is needed.

From the panel-dpi DT binding:

properties:
  compatible:
    description:
      Shall contain a panel specific compatible and "panel-dpi"
      in that order.
    items:
      - {}
      - const: panel-dpi

The panel-specific compatible is mandatory, whether you like it or not.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 15:37, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
>> On 2/23/22 14:47, Maxime Ripard wrote:
>>> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>>>> On 2/23/22 14:41, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>>>
>>>>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>>     drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>>>     1 file changed, 32 insertions(+)
>>>>>>
>>>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>>>
>>>>>> Max
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>>>     	int ret;
>>>>>>     	np = dev->of_node;
>>>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>>>> +	} else {
>>>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>>>> +			np);
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>>>> from the compatible? I'd expect two panels that don't have the same bus
>>>>> format to not be claimed as compatible.
>>>>
>>>> Which compatible ?
>>>>
>>>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>>>> specified in DT (and needs bus format specified there too).
>>>
>>> panel-dpi is supposed to have two compatibles, the panel-specific one
>>> and panel-dpi. This would obviously be tied to the panel-specific one.
>>
>> This whole discussion is about the one which only has 'panel-dpi' compatible
>> and describes the panel in DT completely. The specific compatible is not
>> present in DT when this patch is needed.
> 
>  From the panel-dpi DT binding:
> 
> properties:
>    compatible:
>      description:
>        Shall contain a panel specific compatible and "panel-dpi"
>        in that order.
>      items:
>        - {}
>        - const: panel-dpi
> 
> The panel-specific compatible is mandatory, whether you like it or not.

It doesn't seem to me that's the intended use per panel-simple.c , so 
maybe the bindings need to be fixed too ?

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 15:37, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
>> On 2/23/22 14:47, Maxime Ripard wrote:
>>> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>>>> On 2/23/22 14:41, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>>>
>>>>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>>     drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>>>     1 file changed, 32 insertions(+)
>>>>>>
>>>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>>>
>>>>>> Max
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>>>     	int ret;
>>>>>>     	np = dev->of_node;
>>>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>>>> +		desc->bpc = 6;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>>>> +		desc->bpc = 8;
>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>>>> +	} else {
>>>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>>>> +			np);
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>>>> from the compatible? I'd expect two panels that don't have the same bus
>>>>> format to not be claimed as compatible.
>>>>
>>>> Which compatible ?
>>>>
>>>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>>>> specified in DT (and needs bus format specified there too).
>>>
>>> panel-dpi is supposed to have two compatibles, the panel-specific one
>>> and panel-dpi. This would obviously be tied to the panel-specific one.
>>
>> This whole discussion is about the one which only has 'panel-dpi' compatible
>> and describes the panel in DT completely. The specific compatible is not
>> present in DT when this patch is needed.
> 
>  From the panel-dpi DT binding:
> 
> properties:
>    compatible:
>      description:
>        Shall contain a panel specific compatible and "panel-dpi"
>        in that order.
>      items:
>        - {}
>        - const: panel-dpi
> 
> The panel-specific compatible is mandatory, whether you like it or not.

It doesn't seem to me that's the intended use per panel-simple.c , so 
maybe the bindings need to be fixed too ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

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.

On Wed, Feb 23, 2022 at 2:45 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/23/22 14:41, Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> >> Use the new property bus-format to set the enum bus_format and bpc.
> >> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> >>
> >> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> >>
> >> ---
> >>
> >>   drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> >>   1 file changed, 32 insertions(+)
> >>
> >> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> >>
> >> Max
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> >> index c5f133667a2d..5c07260de71c 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 *format = "";
> >>      int ret;
> >>
> >>      np = dev->of_node;
> >> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> >> +    if (!strcmp(format, "BGR888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> >> +    } else if (!strcmp(format, "GBR888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> >> +    } else if (!strcmp(format, "RBG888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> >> +    } else if (!strcmp(format, "RGB444_1X12")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> >> +    } else if (!strcmp(format, "RGB565_1X16")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> >> +    } else if (!strcmp(format, "RGB666_1X18")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> >> +    } else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> >> +    } else if (!strcmp(format, "RGB888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >> +    } else {
> >> +            dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> >> +                    np);
> >> +            return -EINVAL;
> >> +    }
> >> +
> >
> > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > from the compatible? I'd expect two panels that don't have the same bus
> > format to not be claimed as compatible.
>
> Which compatible ?
>
> Note that this is for panel-dpi compatible, i.e. the panel which has
> timings specified in DT (and needs bus format specified there too).
>
> I agree this doesn't look right however, some more generic color channel
> width/shift/mapping might be better.

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

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.

On Wed, Feb 23, 2022 at 2:45 PM Marek Vasut <marex@denx.de> wrote:
>
> On 2/23/22 14:41, Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> >> Use the new property bus-format to set the enum bus_format and bpc.
> >> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> >>
> >> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> >>
> >> ---
> >>
> >>   drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> >>   1 file changed, 32 insertions(+)
> >>
> >> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> >>
> >> Max
> >>
> >> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> >> index c5f133667a2d..5c07260de71c 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 *format = "";
> >>      int ret;
> >>
> >>      np = dev->of_node;
> >> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> >> +    if (!strcmp(format, "BGR888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> >> +    } else if (!strcmp(format, "GBR888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> >> +    } else if (!strcmp(format, "RBG888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> >> +    } else if (!strcmp(format, "RGB444_1X12")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> >> +    } else if (!strcmp(format, "RGB565_1X16")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> >> +    } else if (!strcmp(format, "RGB666_1X18")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> >> +    } else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> >> +            desc->bpc = 6;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> >> +    } else if (!strcmp(format, "RGB888_1X24")) {
> >> +            desc->bpc = 8;
> >> +            desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >> +    } else {
> >> +            dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> >> +                    np);
> >> +            return -EINVAL;
> >> +    }
> >> +
> >
> > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > from the compatible? I'd expect two panels that don't have the same bus
> > format to not be claimed as compatible.
>
> Which compatible ?
>
> Note that this is for panel-dpi compatible, i.e. the panel which has
> timings specified in DT (and needs bus format specified there too).
>
> I agree this doesn't look right however, some more generic color channel
> width/shift/mapping might be better.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]

On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote:
> On 2/23/22 15:37, Maxime Ripard wrote:
> > On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
> > > On 2/23/22 14:47, Maxime Ripard wrote:
> > > > On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
> > > > > On 2/23/22 14:41, Maxime Ripard wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> > > > > > > Use the new property bus-format to set the enum bus_format and bpc.
> > > > > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> > > > > > > 
> > > > > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > >     drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> > > > > > >     1 file changed, 32 insertions(+)
> > > > > > > 
> > > > > > > Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> > > > > > > 
> > > > > > > Max
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > > > > index c5f133667a2d..5c07260de71c 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 *format = "";
> > > > > > >     	int ret;
> > > > > > >     	np = dev->of_node;
> > > > > > > @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> > > > > > > +	if (!strcmp(format, "BGR888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > > > > > > +	} else if (!strcmp(format, "GBR888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > > > > > > +	} else if (!strcmp(format, "RBG888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > > > > > > +	} else if (!strcmp(format, "RGB444_1X12")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> > > > > > > +	} else if (!strcmp(format, "RGB565_1X16")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > > > > +	} else if (!strcmp(format, "RGB666_1X18")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > > > > +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > > > > > +	} else if (!strcmp(format, "RGB888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > > > > +	} else {
> > > > > > > +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> > > > > > > +			np);
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > 
> > > > > > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > > > > > from the compatible? I'd expect two panels that don't have the same bus
> > > > > > format to not be claimed as compatible.
> > > > > 
> > > > > Which compatible ?
> > > > > 
> > > > > Note that this is for panel-dpi compatible, i.e. the panel which has timings
> > > > > specified in DT (and needs bus format specified there too).
> > > > 
> > > > panel-dpi is supposed to have two compatibles, the panel-specific one
> > > > and panel-dpi. This would obviously be tied to the panel-specific one.
> > > 
> > > This whole discussion is about the one which only has 'panel-dpi' compatible
> > > and describes the panel in DT completely. The specific compatible is not
> > > present in DT when this patch is needed.
> > 
> >  From the panel-dpi DT binding:
> > 
> > properties:
> >    compatible:
> >      description:
> >        Shall contain a panel specific compatible and "panel-dpi"
> >        in that order.
> >      items:
> >        - {}
> >        - const: panel-dpi
> > 
> > The panel-specific compatible is mandatory, whether you like it or not.
> 
> It doesn't seem to me that's the intended use per panel-simple.c , so maybe
> the bindings need to be fixed too ?

It's not clear to me why this has anything to do with panel-simple, but
the binding is correct, and that requirement is fairly standard. We have
the same thing with panel-lvds for example.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 5004 bytes --]

On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote:
> On 2/23/22 15:37, Maxime Ripard wrote:
> > On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
> > > On 2/23/22 14:47, Maxime Ripard wrote:
> > > > On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
> > > > > On 2/23/22 14:41, Maxime Ripard wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
> > > > > > > Use the new property bus-format to set the enum bus_format and bpc.
> > > > > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
> > > > > > > 
> > > > > > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > >     drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
> > > > > > >     1 file changed, 32 insertions(+)
> > > > > > > 
> > > > > > > Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
> > > > > > > 
> > > > > > > Max
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > > > > > index c5f133667a2d..5c07260de71c 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 *format = "";
> > > > > > >     	int ret;
> > > > > > >     	np = dev->of_node;
> > > > > > > @@ -477,6 +478,37 @@ 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, "bus-format", &format);
> > > > > > > +	if (!strcmp(format, "BGR888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
> > > > > > > +	} else if (!strcmp(format, "GBR888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
> > > > > > > +	} else if (!strcmp(format, "RBG888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
> > > > > > > +	} else if (!strcmp(format, "RGB444_1X12")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
> > > > > > > +	} else if (!strcmp(format, "RGB565_1X16")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
> > > > > > > +	} else if (!strcmp(format, "RGB666_1X18")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
> > > > > > > +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
> > > > > > > +		desc->bpc = 6;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
> > > > > > > +	} else if (!strcmp(format, "RGB888_1X24")) {
> > > > > > > +		desc->bpc = 8;
> > > > > > > +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > > > > > > +	} else {
> > > > > > > +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
> > > > > > > +			np);
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > 
> > > > > > It doesn't seem right, really. We can't the bus format / bpc be inferred
> > > > > > from the compatible? I'd expect two panels that don't have the same bus
> > > > > > format to not be claimed as compatible.
> > > > > 
> > > > > Which compatible ?
> > > > > 
> > > > > Note that this is for panel-dpi compatible, i.e. the panel which has timings
> > > > > specified in DT (and needs bus format specified there too).
> > > > 
> > > > panel-dpi is supposed to have two compatibles, the panel-specific one
> > > > and panel-dpi. This would obviously be tied to the panel-specific one.
> > > 
> > > This whole discussion is about the one which only has 'panel-dpi' compatible
> > > and describes the panel in DT completely. The specific compatible is not
> > > present in DT when this patch is needed.
> > 
> >  From the panel-dpi DT binding:
> > 
> > properties:
> >    compatible:
> >      description:
> >        Shall contain a panel specific compatible and "panel-dpi"
> >        in that order.
> >      items:
> >        - {}
> >        - const: panel-dpi
> > 
> > The panel-specific compatible is mandatory, whether you like it or not.
> 
> It doesn't seem to me that's the intended use per panel-simple.c , so maybe
> the bindings need to be fixed too ?

It's not clear to me why this has anything to do with panel-simple, but
the binding is correct, and that requirement is fairly standard. We have
the same thing with panel-lvds for example.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 17:39, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote:
>> On 2/23/22 15:37, Maxime Ripard wrote:
>>> On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
>>>> On 2/23/22 14:47, Maxime Ripard wrote:
>>>>> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>>>>>> On 2/23/22 14:41, Maxime Ripard wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>>>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>>>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>>>>>
>>>>>>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 32 insertions(+)
>>>>>>>>
>>>>>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>>>>>
>>>>>>>> Max
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>>>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>>>>>      	int ret;
>>>>>>>>      	np = dev->of_node;
>>>>>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>>>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>>>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>>>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>>>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>>>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>>>>>> +	} else {
>>>>>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>>>>>> +			np);
>>>>>>>> +		return -EINVAL;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>>>>>> from the compatible? I'd expect two panels that don't have the same bus
>>>>>>> format to not be claimed as compatible.
>>>>>>
>>>>>> Which compatible ?
>>>>>>
>>>>>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>>>>>> specified in DT (and needs bus format specified there too).
>>>>>
>>>>> panel-dpi is supposed to have two compatibles, the panel-specific one
>>>>> and panel-dpi. This would obviously be tied to the panel-specific one.
>>>>
>>>> This whole discussion is about the one which only has 'panel-dpi' compatible
>>>> and describes the panel in DT completely. The specific compatible is not
>>>> present in DT when this patch is needed.
>>>
>>>   From the panel-dpi DT binding:
>>>
>>> properties:
>>>     compatible:
>>>       description:
>>>         Shall contain a panel specific compatible and "panel-dpi"
>>>         in that order.
>>>       items:
>>>         - {}
>>>         - const: panel-dpi
>>>
>>> The panel-specific compatible is mandatory, whether you like it or not.
>>
>> It doesn't seem to me that's the intended use per panel-simple.c , so maybe
>> the bindings need to be fixed too ?
> 
> It's not clear to me why this has anything to do with panel-simple, but
> the binding is correct, and that requirement is fairly standard. We have
> the same thing with panel-lvds for example.

I think this patch is related to this patch, which was not mentioned in 
the commit message:

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

(unless I am confused)

With LVDS the situation is simpler, you have three formats and that's it 
(18bpp and 2 24bpp), with DPI it is more complex, since you need to deal 
with color channel width (888, 666 and even 565 and other oddities), 
then with mapping (RGB, BGR, etc), and then you can have panels with 
only 18bpp inputs wired to 24bpp DPI bus and vice versa which you also 
have to somehow describe.

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 2/23/22 17:39, Maxime Ripard wrote:
> On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote:
>> On 2/23/22 15:37, Maxime Ripard wrote:
>>> On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote:
>>>> On 2/23/22 14:47, Maxime Ripard wrote:
>>>>> On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote:
>>>>>> On 2/23/22 14:41, Maxime Ripard wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote:
>>>>>>>> Use the new property bus-format to set the enum bus_format and bpc.
>>>>>>>> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support")
>>>>>>>>
>>>>>>>> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>      drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 32 insertions(+)
>>>>>>>>
>>>>>>>> Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniedermaier@dh-electronics.com/
>>>>>>>>
>>>>>>>> Max
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
>>>>>>>> index c5f133667a2d..5c07260de71c 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 *format = "";
>>>>>>>>      	int ret;
>>>>>>>>      	np = dev->of_node;
>>>>>>>> @@ -477,6 +478,37 @@ 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, "bus-format", &format);
>>>>>>>> +	if (!strcmp(format, "BGR888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "GBR888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "RBG888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24;
>>>>>>>> +	} else if (!strcmp(format, "RGB444_1X12")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12;
>>>>>>>> +	} else if (!strcmp(format, "RGB565_1X16")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16;
>>>>>>>> +	} else if (!strcmp(format, "RGB666_1X18")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18;
>>>>>>>> +	} else if (!strcmp(format, "RGB666_1X24_CPADHI")) {
>>>>>>>> +		desc->bpc = 6;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
>>>>>>>> +	} else if (!strcmp(format, "RGB888_1X24")) {
>>>>>>>> +		desc->bpc = 8;
>>>>>>>> +		desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>>>>>>>> +	} else {
>>>>>>>> +		dev_err(dev, "%pOF: missing or unknown bus-format property\n",
>>>>>>>> +			np);
>>>>>>>> +		return -EINVAL;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> It doesn't seem right, really. We can't the bus format / bpc be inferred
>>>>>>> from the compatible? I'd expect two panels that don't have the same bus
>>>>>>> format to not be claimed as compatible.
>>>>>>
>>>>>> Which compatible ?
>>>>>>
>>>>>> Note that this is for panel-dpi compatible, i.e. the panel which has timings
>>>>>> specified in DT (and needs bus format specified there too).
>>>>>
>>>>> panel-dpi is supposed to have two compatibles, the panel-specific one
>>>>> and panel-dpi. This would obviously be tied to the panel-specific one.
>>>>
>>>> This whole discussion is about the one which only has 'panel-dpi' compatible
>>>> and describes the panel in DT completely. The specific compatible is not
>>>> present in DT when this patch is needed.
>>>
>>>   From the panel-dpi DT binding:
>>>
>>> properties:
>>>     compatible:
>>>       description:
>>>         Shall contain a panel specific compatible and "panel-dpi"
>>>         in that order.
>>>       items:
>>>         - {}
>>>         - const: panel-dpi
>>>
>>> The panel-specific compatible is mandatory, whether you like it or not.
>>
>> It doesn't seem to me that's the intended use per panel-simple.c , so maybe
>> the bindings need to be fixed too ?
> 
> It's not clear to me why this has anything to do with panel-simple, but
> the binding is correct, and that requirement is fairly standard. We have
> the same thing with panel-lvds for example.

I think this patch is related to this patch, which was not mentioned in 
the commit message:

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

(unless I am confused)

With LVDS the situation is simpler, you have three formats and that's it 
(18bpp and 2 24bpp), with DPI it is more complex, since you need to deal 
with color channel width (888, 666 and even 565 and other oddities), 
then with mapping (RGB, BGR, etc), and then you can have panels with 
only 18bpp inputs wired to 24bpp DPI bus and vice versa which you also 
have to somehow describe.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

Hi,

Please try to avoid top posting

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.

That being said, what I (and I can only assume Marek) don't like is the
string encoding. Especially when the similar bus-type property uses a
integer with the various available bus options we have.

Having an integer, with a set of defines that you would map to the
proper MEDIA_BUS_* would be more efficient and more elegant.

That being said, the first question that needs to be answered is why
does this have to be in the DT in the first place?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1353 bytes --]

Hi,

Please try to avoid top posting

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.

That being said, what I (and I can only assume Marek) don't like is the
string encoding. Especially when the similar bus-type property uses a
integer with the various available bus options we have.

Having an integer, with a set of defines that you would map to the
proper MEDIA_BUS_* would be more efficient and more elegant.

That being said, the first question that needs to be answered is why
does this have to be in the DT in the first place?

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 3/2/22 15:21, Maxime Ripard wrote:
> Hi,

Hi,

> Please try to avoid top posting
> 
> 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 ?

> That being said, what I (and I can only assume Marek) don't like is the
> string encoding. Especially when the similar bus-type property uses a
> integer with the various available bus options we have.

Right, the string encoding isn't good.

> Having an integer, with a set of defines that you would map to the
> proper MEDIA_BUS_* would be more efficient and more elegant.
> 
> That being said, the first question that needs to be answered is why
> does this have to be in the DT in the first place?

Because panel-simple panel-dpi , you may need to specify the bus format 
between the last bridge and the panel .

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Marek Vasut]

On 3/2/22 15:21, Maxime Ripard wrote:
> Hi,

Hi,

> Please try to avoid top posting
> 
> 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 ?

> That being said, what I (and I can only assume Marek) don't like is the
> string encoding. Especially when the similar bus-type property uses a
> integer with the various available bus options we have.

Right, the string encoding isn't good.

> Having an integer, with a set of defines that you would map to the
> proper MEDIA_BUS_* would be more efficient and more elegant.
> 
> That being said, the first question that needs to be answered is why
> does this have to be in the DT in the first place?

Because panel-simple panel-dpi , you may need to specify the bus format 
between the last bridge and the panel .

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

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.

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.

>
> > That being said, what I (and I can only assume Marek) don't like is the
> > string encoding. Especially when the similar bus-type property uses a
> > integer with the various available bus options we have.
>
> Right, the string encoding isn't good.
>
> > Having an integer, with a set of defines that you would map to the
> > proper MEDIA_BUS_* would be more efficient and more elegant.

I have a look at that.

> >
> > That being said, the first question that needs to be answered is why
> > does this have to be in the DT in the first place?

The way I understand the compatible panel-dp, iti should allow to fill
a 'struct panel_desc'
with data provided by the device tree rather than having the info
hardcoded in the C source.
The missing element is bus_format which currently is kept at 0.

>
> Because panel-simple panel-dpi , you may need to specify the bus format
> between the last bridge and the panel .

Exactly.

Max

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

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.

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.

>
> > That being said, what I (and I can only assume Marek) don't like is the
> > string encoding. Especially when the similar bus-type property uses a
> > integer with the various available bus options we have.
>
> Right, the string encoding isn't good.
>
> > Having an integer, with a set of defines that you would map to the
> > proper MEDIA_BUS_* would be more efficient and more elegant.

I have a look at that.

> >
> > That being said, the first question that needs to be answered is why
> > does this have to be in the DT in the first place?

The way I understand the compatible panel-dp, iti should allow to fill
a 'struct panel_desc'
with data provided by the device tree rather than having the info
hardcoded in the C source.
The missing element is bus_format which currently is kept at 0.

>
> Because panel-simple panel-dpi , you may need to specify the bus format
> between the last bridge and the panel .

Exactly.

Max

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3401 bytes --]

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.

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3401 bytes --]

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.

> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Dave Stevenson]

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

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Dave Stevenson]

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

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4671 bytes --]

On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> 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.

Yeah, that's kind of my point actually :)

Just having a rgb666 string won't allow to differentiate between
MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
string but that usually leads to inconsistent or weird names, so this
isn't ideal.

> 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).

I think having an integer value is indeed better: it doesn't change much
in the device tree if we're using a header, it makes the driver simpler
since we don't have to parse a string, and we can easily extend it or
rename the define, it won't change the ABI.

I'm not sure using the raw media bus format value is ideal though, since
that value could then be used by any OS, and it would effectively force
the mbus stuff down their throat.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4671 bytes --]

On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> 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.

Yeah, that's kind of my point actually :)

Just having a rgb666 string won't allow to differentiate between
MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
string but that usually leads to inconsistent or weird names, so this
isn't ideal.

> 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).

I think having an integer value is indeed better: it doesn't change much
in the device tree if we're using a header, it makes the driver simpler
since we don't have to parse a string, and we can easily extend it or
rename the define, it won't change the ABI.

I'm not sure using the raw media bus format value is ideal though, since
that value could then be used by any OS, and it would effectively force
the mbus stuff down their throat.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Dave Stevenson]

On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > 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.
>
> Yeah, that's kind of my point actually :)

Ah, OK :)

> Just having a rgb666 string won't allow to differentiate between
> MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> string but that usually leads to inconsistent or weird names, so this
> isn't ideal.
>
> > 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).
>
> I think having an integer value is indeed better: it doesn't change much
> in the device tree if we're using a header, it makes the driver simpler
> since we don't have to parse a string, and we can easily extend it or
> rename the define, it won't change the ABI.
>
> I'm not sure using the raw media bus format value is ideal though, since
> that value could then be used by any OS, and it would effectively force
> the mbus stuff down their throat.

I'll agree that the media bus format isn't the nicest, but I was
looking for a quick fix that could be configured from an overlay.

If using defines, then possibly go for a partial bitmask?
3 bits for RGB order can be defined across the board. An encoding of
the bus width. And then the packing within that bus width would have
to be a lookup table, with no padding, padhi, and padlo being defined
as 0, 1, and 2 respectively. >=3 are extensions per bus width.
MEDIA_BUS_FMT_RGB666_1X24_CPADHI might then be described as ORDER_RGB
| BUS_24 | PAD_HI.
And MEDIA_BUS_FMT_BGR666 as ORDER_BGR | BUS_18 | NO_PAD.

Hmm, a bit more thought needed for RGB565, as a bus width of 16
wouldn't guarantee that.

  Dave

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Dave Stevenson]

On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > 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.
>
> Yeah, that's kind of my point actually :)

Ah, OK :)

> Just having a rgb666 string won't allow to differentiate between
> MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> string but that usually leads to inconsistent or weird names, so this
> isn't ideal.
>
> > 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).
>
> I think having an integer value is indeed better: it doesn't change much
> in the device tree if we're using a header, it makes the driver simpler
> since we don't have to parse a string, and we can easily extend it or
> rename the define, it won't change the ABI.
>
> I'm not sure using the raw media bus format value is ideal though, since
> that value could then be used by any OS, and it would effectively force
> the mbus stuff down their throat.

I'll agree that the media bus format isn't the nicest, but I was
looking for a quick fix that could be configured from an overlay.

If using defines, then possibly go for a partial bitmask?
3 bits for RGB order can be defined across the board. An encoding of
the bus width. And then the packing within that bus width would have
to be a lookup table, with no padding, padhi, and padlo being defined
as 0, 1, and 2 respectively. >=3 are extensions per bus width.
MEDIA_BUS_FMT_RGB666_1X24_CPADHI might then be described as ORDER_RGB
| BUS_24 | PAD_HI.
And MEDIA_BUS_FMT_BGR666 as ORDER_BGR | BUS_18 | NO_PAD.

Hmm, a bit more thought needed for RGB565, as a bus width of 16
wouldn't guarantee that.

  Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
> > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > 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.

I agree that the strings used to set "data-mapping" weren't self explaining.
However, as there was a
clear 1:1 relation to the bus_format value the meaning
wasn't ambiguous at all.

> > > 
> > > 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.
> > 
> > Yeah, that's kind of my point actually :)
> 
> Ah, OK :)
> 
> > Just having a rgb666 string won't allow to differentiate between
> > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > string but that usually leads to inconsistent or weird names, so this
> > isn't ideal.

We're on the same page that the strings that were used aren't self
explaining and do not follow a pattern which would make it easy to
extend. However that is something I addressed in my RFC proposal, not?

> > 
> > > 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).
> > 
> > I think having an integer value is indeed better: it doesn't change much
> > in the device tree if we're using a header, it makes the driver simpler
> > since we don't have to parse a string, and we can easily extend it or
> > rename the define, it won't change the ABI.

Fine with me.

> > 
> > I'm not sure using the raw media bus format value is ideal though, since
> > that value could then be used by any OS, and it would effectively force
> > the mbus stuff down their throat.

I disagree here, this forces us to use code to map the device tree enum
to the kernel enum for Linux, i.e. adds complexity and maintenance work
if additional bus_formats are needed.
Assuming there is another OS which uses the device tree it would not
make a difference, that OS would still need to map the device tree enum
to the corresponding representation in their kernel.
I would copy the definitions of media-bus-format.h into a header in
include/dt-bindings similarly as it is done for
include/dt-bindings/display/sdtv-standards.h for TV standards.

> 
> I'll agree that the media bus format isn't the nicest, but I was
> looking for a quick fix that could be configured from an overlay.
> 
> If using defines, then possibly go for a partial bitmask?
> 3 bits for RGB order can be defined across the board. An encoding of
> the bus width. And then the packing within that bus width would have
> to be a lookup table, with no padding, padhi, and padlo being defined
> as 0, 1, and 2 respectively. >=3 are extensions per bus width.
> MEDIA_BUS_FMT_RGB666_1X24_CPADHI might then be described as ORDER_RGB
> > BUS_24 | PAD_HI.
> And MEDIA_BUS_FMT_BGR666 as ORDER_BGR | BUS_18 | NO_PAD.
> 
> Hmm, a bit more thought needed for RGB565, as a bus width of 16
> wouldn't guarantee that.

I disagree here, I don't see value in that structuring. It won't
help us mapping it to the corresponding bus_format enum and it
might be incomplete for bus_formats added in the future. 
E.g. besides your RGB565 example consider a Tegra 30 which for RGB666
outputs them on [23:8] and for RGB888 the 6 most significant bits are
kept in [23:8], the 2 least significant ones in [7:0]. That wouldn't
fit in this structured enum you propose, however one could easily add
a new consecutive number to the enum.

Max

> 
>   Dave


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
> > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > 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.

I agree that the strings used to set "data-mapping" weren't self explaining.
However, as there was a
clear 1:1 relation to the bus_format value the meaning
wasn't ambiguous at all.

> > > 
> > > 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.
> > 
> > Yeah, that's kind of my point actually :)
> 
> Ah, OK :)
> 
> > Just having a rgb666 string won't allow to differentiate between
> > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > string but that usually leads to inconsistent or weird names, so this
> > isn't ideal.

We're on the same page that the strings that were used aren't self
explaining and do not follow a pattern which would make it easy to
extend. However that is something I addressed in my RFC proposal, not?

> > 
> > > 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).
> > 
> > I think having an integer value is indeed better: it doesn't change much
> > in the device tree if we're using a header, it makes the driver simpler
> > since we don't have to parse a string, and we can easily extend it or
> > rename the define, it won't change the ABI.

Fine with me.

> > 
> > I'm not sure using the raw media bus format value is ideal though, since
> > that value could then be used by any OS, and it would effectively force
> > the mbus stuff down their throat.

I disagree here, this forces us to use code to map the device tree enum
to the kernel enum for Linux, i.e. adds complexity and maintenance work
if additional bus_formats are needed.
Assuming there is another OS which uses the device tree it would not
make a difference, that OS would still need to map the device tree enum
to the corresponding representation in their kernel.
I would copy the definitions of media-bus-format.h into a header in
include/dt-bindings similarly as it is done for
include/dt-bindings/display/sdtv-standards.h for TV standards.

> 
> I'll agree that the media bus format isn't the nicest, but I was
> looking for a quick fix that could be configured from an overlay.
> 
> If using defines, then possibly go for a partial bitmask?
> 3 bits for RGB order can be defined across the board. An encoding of
> the bus width. And then the packing within that bus width would have
> to be a lookup table, with no padding, padhi, and padlo being defined
> as 0, 1, and 2 respectively. >=3 are extensions per bus width.
> MEDIA_BUS_FMT_RGB666_1X24_CPADHI might then be described as ORDER_RGB
> > BUS_24 | PAD_HI.
> And MEDIA_BUS_FMT_BGR666 as ORDER_BGR | BUS_18 | NO_PAD.
> 
> Hmm, a bit more thought needed for RGB565, as a bus width of 16
> wouldn't guarantee that.

I disagree here, I don't see value in that structuring. It won't
help us mapping it to the corresponding bus_format enum and it
might be incomplete for bus_formats added in the future. 
E.g. besides your RGB565 example consider a Tegra 30 which for RGB666
outputs them on [23:8] and for RGB888 the 6 most significant bits are
kept in [23:8], the 2 least significant ones in [7:0]. That wouldn't
fit in this structured enum you propose, however one could easily add
a new consecutive number to the enum.

Max

> 
>   Dave

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6897 bytes --]

On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > 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.
> 
> I agree that the strings used to set "data-mapping" weren't self explaining.
> However, as there was a
> clear 1:1 relation to the bus_format value the meaning
> wasn't ambiguous at all.
> 
> > > > 
> > > > 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.
> > > 
> > > Yeah, that's kind of my point actually :)
> > 
> > Ah, OK :)
> > 
> > > Just having a rgb666 string won't allow to differentiate between
> > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > string but that usually leads to inconsistent or weird names, so this
> > > isn't ideal.
> 
> We're on the same page that the strings that were used aren't self
> explaining and do not follow a pattern which would make it easy to
> extend. However that is something I addressed in my RFC proposal, not?
> 
> > > 
> > > > 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).
> > > 
> > > I think having an integer value is indeed better: it doesn't change much
> > > in the device tree if we're using a header, it makes the driver simpler
> > > since we don't have to parse a string, and we can easily extend it or
> > > rename the define, it won't change the ABI.
> 
> Fine with me.
> 
> > > 
> > > I'm not sure using the raw media bus format value is ideal though, since
> > > that value could then be used by any OS, and it would effectively force
> > > the mbus stuff down their throat.
> 
> I disagree here, this forces us to use code to map the device tree enum
> to the kernel enum for Linux, i.e. adds complexity and maintenance work
> if additional bus_formats are needed.
> Assuming there is another OS which uses the device tree it would not
> make a difference, that OS would still need to map the device tree enum
> to the corresponding representation in their kernel.

So, you don't want to do something in Linux, but would expect someone
else to be completely ok with that?

> I would copy the definitions of media-bus-format.h into a header in
> include/dt-bindings similarly as it is done for
> include/dt-bindings/display/sdtv-standards.h for TV standards.

That might not be an option: that header is licensed under the GPL,
device trees are usually licensed under GPL+MIT, and we don't have any
requirements on the license for other projects using a DT (hence the
dual license).

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 6897 bytes --]

On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
> > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > 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.
> 
> I agree that the strings used to set "data-mapping" weren't self explaining.
> However, as there was a
> clear 1:1 relation to the bus_format value the meaning
> wasn't ambiguous at all.
> 
> > > > 
> > > > 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.
> > > 
> > > Yeah, that's kind of my point actually :)
> > 
> > Ah, OK :)
> > 
> > > Just having a rgb666 string won't allow to differentiate between
> > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > string but that usually leads to inconsistent or weird names, so this
> > > isn't ideal.
> 
> We're on the same page that the strings that were used aren't self
> explaining and do not follow a pattern which would make it easy to
> extend. However that is something I addressed in my RFC proposal, not?
> 
> > > 
> > > > 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).
> > > 
> > > I think having an integer value is indeed better: it doesn't change much
> > > in the device tree if we're using a header, it makes the driver simpler
> > > since we don't have to parse a string, and we can easily extend it or
> > > rename the define, it won't change the ABI.
> 
> Fine with me.
> 
> > > 
> > > I'm not sure using the raw media bus format value is ideal though, since
> > > that value could then be used by any OS, and it would effectively force
> > > the mbus stuff down their throat.
> 
> I disagree here, this forces us to use code to map the device tree enum
> to the kernel enum for Linux, i.e. adds complexity and maintenance work
> if additional bus_formats are needed.
> Assuming there is another OS which uses the device tree it would not
> make a difference, that OS would still need to map the device tree enum
> to the corresponding representation in their kernel.

So, you don't want to do something in Linux, but would expect someone
else to be completely ok with that?

> I would copy the definitions of media-bus-format.h into a header in
> include/dt-bindings similarly as it is done for
> include/dt-bindings/display/sdtv-standards.h for TV standards.

That might not be an option: that header is licensed under the GPL,
device trees are usually licensed under GPL+MIT, and we don't have any
requirements on the license for other projects using a DT (hence the
dual license).

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > > 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.
> > 
> > I agree that the strings used to set "data-mapping" weren't self explaining.
> > However, as there was a
> > clear 1:1 relation to the bus_format value the meaning
> > wasn't ambiguous at all.
> > 
> > > > > 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.
> > > > 
> > > > Yeah, that's kind of my point actually :)
> > > 
> > > Ah, OK :)
> > > 
> > > > Just having a rgb666 string won't allow to differentiate between
> > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > > string but that usually leads to inconsistent or weird names, so this
> > > > isn't ideal.
> > 
> > We're on the same page that the strings that were used aren't self
> > explaining and do not follow a pattern which would make it easy to
> > extend. However that is something I addressed in my RFC proposal, not?
> > 
> > > > > 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).
> > > > 
> > > > I think having an integer value is indeed better: it doesn't change much
> > > > in the device tree if we're using a header, it makes the driver simpler
> > > > since we don't have to parse a string, and we can easily extend it or
> > > > rename the define, it won't change the ABI.
> > 
> > Fine with me.
> > 
> > > > I'm not sure using the raw media bus format value is ideal though, since
> > > > that value could then be used by any OS, and it would effectively force
> > > > the mbus stuff down their throat.
> > 
> > I disagree here, this forces us to use code to map the device tree enum
> > to the kernel enum for Linux, i.e. adds complexity and maintenance work
> > if additional bus_formats are needed.
> > Assuming there is another OS which uses the device tree it would not
> > make a difference, that OS would still need to map the device tree enum
> > to the corresponding representation in their kernel.
> 
> So, you don't want to do something in Linux, but would expect someone
> else to be completely ok with that?

Yes, sort of.
Recycling the values as used currently in the Linux kernel rather than
inventing a new numbering will make the Linux code a little easier to
write, read and maintain without any negative effect on how that other
OSs would have to map the DT representation to their internal representation.

Would you rather have something like:

<the common dt-bindings header file>
DT_BUS_FMT_RGB666_1X18	1
DT_BUS_FMT_RGB888_1X24	2
...

<panel-simple.c>
switch (bus-format) {
case DT_BUS_FMT_RGB666_1X18:
  bus_format = MEDIA_BUS_FMT_RGB666_1X18; break;
case DT_BUS_FMT_RGB888_1X24:
  bus_format = MEDIA_BUS_FMT_RGB888_1X24; break;
...

> 
> > I would copy the definitions of media-bus-format.h into a header in
> > include/dt-bindings similarly as it is done for
> > include/dt-bindings/display/sdtv-standards.h for TV standards.
> 
> That might not be an option: that header is licensed under the GPL,
> device trees are usually licensed under GPL+MIT, and we don't have any
> requirements on the license for other projects using a DT (hence the
> dual license).

That one I didn't consider. That would be solved by a newly invented
enum.

Max

> 
> Maxime

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > > 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.
> > 
> > I agree that the strings used to set "data-mapping" weren't self explaining.
> > However, as there was a
> > clear 1:1 relation to the bus_format value the meaning
> > wasn't ambiguous at all.
> > 
> > > > > 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.
> > > > 
> > > > Yeah, that's kind of my point actually :)
> > > 
> > > Ah, OK :)
> > > 
> > > > Just having a rgb666 string won't allow to differentiate between
> > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > > string but that usually leads to inconsistent or weird names, so this
> > > > isn't ideal.
> > 
> > We're on the same page that the strings that were used aren't self
> > explaining and do not follow a pattern which would make it easy to
> > extend. However that is something I addressed in my RFC proposal, not?
> > 
> > > > > 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).
> > > > 
> > > > I think having an integer value is indeed better: it doesn't change much
> > > > in the device tree if we're using a header, it makes the driver simpler
> > > > since we don't have to parse a string, and we can easily extend it or
> > > > rename the define, it won't change the ABI.
> > 
> > Fine with me.
> > 
> > > > I'm not sure using the raw media bus format value is ideal though, since
> > > > that value could then be used by any OS, and it would effectively force
> > > > the mbus stuff down their throat.
> > 
> > I disagree here, this forces us to use code to map the device tree enum
> > to the kernel enum for Linux, i.e. adds complexity and maintenance work
> > if additional bus_formats are needed.
> > Assuming there is another OS which uses the device tree it would not
> > make a difference, that OS would still need to map the device tree enum
> > to the corresponding representation in their kernel.
> 
> So, you don't want to do something in Linux, but would expect someone
> else to be completely ok with that?

Yes, sort of.
Recycling the values as used currently in the Linux kernel rather than
inventing a new numbering will make the Linux code a little easier to
write, read and maintain without any negative effect on how that other
OSs would have to map the DT representation to their internal representation.

Would you rather have something like:

<the common dt-bindings header file>
DT_BUS_FMT_RGB666_1X18	1
DT_BUS_FMT_RGB888_1X24	2
...

<panel-simple.c>
switch (bus-format) {
case DT_BUS_FMT_RGB666_1X18:
  bus_format = MEDIA_BUS_FMT_RGB666_1X18; break;
case DT_BUS_FMT_RGB888_1X24:
  bus_format = MEDIA_BUS_FMT_RGB888_1X24; break;
...

> 
> > I would copy the definitions of media-bus-format.h into a header in
> > include/dt-bindings similarly as it is done for
> > include/dt-bindings/display/sdtv-standards.h for TV standards.
> 
> That might not be an option: that header is licensed under the GPL,
> device trees are usually licensed under GPL+MIT, and we don't have any
> requirements on the license for other projects using a DT (hence the
> dual license).

That one I didn't consider. That would be solved by a newly invented
enum.

Max

> 
> Maxime


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Francesco Dolcini]

On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > I would copy the definitions of media-bus-format.h into a header in
> > > include/dt-bindings similarly as it is done for
> > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > 
> > That might not be an option: that header is licensed under the GPL,
> > device trees are usually licensed under GPL+MIT, and we don't have any
> > requirements on the license for other projects using a DT (hence the
> > dual license).
> 
> That one I didn't consider. That would be solved by a newly invented
> enum.

IANAL, but we are talking about the copyright of something that is not
even a complete API, it is just a list of name/value. I do not believe
that this is a real problem without solution.

Francesco

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Francesco Dolcini]

On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > I would copy the definitions of media-bus-format.h into a header in
> > > include/dt-bindings similarly as it is done for
> > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > 
> > That might not be an option: that header is licensed under the GPL,
> > device trees are usually licensed under GPL+MIT, and we don't have any
> > requirements on the license for other projects using a DT (hence the
> > dual license).
> 
> That one I didn't consider. That would be solved by a newly invented
> enum.

IANAL, but we are talking about the copyright of something that is not
even a complete API, it is just a list of name/value. I do not believe
that this is a real problem without solution.

Francesco

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Laurent Pinchart]

On Thu, Mar 24, 2022 at 09:15:33AM +0100, Francesco Dolcini wrote:
> On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> > Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > > I would copy the definitions of media-bus-format.h into a header in
> > > > include/dt-bindings similarly as it is done for
> > > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > > 
> > > That might not be an option: that header is licensed under the GPL,
> > > device trees are usually licensed under GPL+MIT, and we don't have any
> > > requirements on the license for other projects using a DT (hence the
> > > dual license).
> > 
> > That one I didn't consider. That would be solved by a newly invented
> > enum.
> 
> IANAL, but we are talking about the copyright of something that is not
> even a complete API, it is just a list of name/value. I do not believe
> that this is a real problem without solution.

I agree here, I don't think it's an issue.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Laurent Pinchart]

On Thu, Mar 24, 2022 at 09:15:33AM +0100, Francesco Dolcini wrote:
> On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> > Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > > I would copy the definitions of media-bus-format.h into a header in
> > > > include/dt-bindings similarly as it is done for
> > > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > > 
> > > That might not be an option: that header is licensed under the GPL,
> > > device trees are usually licensed under GPL+MIT, and we don't have any
> > > requirements on the license for other projects using a DT (hence the
> > > dual license).
> > 
> > That one I didn't consider. That would be solved by a newly invented
> > enum.
> 
> IANAL, but we are talking about the copyright of something that is not
> even a complete API, it is just a list of name/value. I do not believe
> that this is a real problem without solution.

I agree here, I don't think it's an issue.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Laurent Pinchart]

On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard wrote:
> > > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > > > On Fri, 18 Mar 2022 at 16:35, Maxime Ripard 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 wrote:
> > > > > > > > > On 3/2/22 15:21, Maxime Ripard wrote:
> > > > > > > > > > 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.
> > > 
> > > I agree that the strings used to set "data-mapping" weren't self explaining.
> > > However, as there was a
> > > clear 1:1 relation to the bus_format value the meaning
> > > wasn't ambiguous at all.
> > > 
> > > > > > 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.
> > > > > 
> > > > > Yeah, that's kind of my point actually :)
> > > > 
> > > > Ah, OK :)
> > > > 
> > > > > Just having a rgb666 string won't allow to differentiate between
> > > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > > > string but that usually leads to inconsistent or weird names, so this
> > > > > isn't ideal.
> > > 
> > > We're on the same page that the strings that were used aren't self
> > > explaining and do not follow a pattern which would make it easy to
> > > extend. However that is something I addressed in my RFC proposal, not?
> > > 
> > > > > > 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).
> > > > > 
> > > > > I think having an integer value is indeed better: it doesn't change much
> > > > > in the device tree if we're using a header, it makes the driver simpler
> > > > > since we don't have to parse a string, and we can easily extend it or
> > > > > rename the define, it won't change the ABI.
> > > 
> > > Fine with me.

I'm fine with integers too. Strings give the false impression that new
values can be added with a lower risk of a conflict, but that just a
false impression.

> > > > > I'm not sure using the raw media bus format value is ideal though, since
> > > > > that value could then be used by any OS, and it would effectively force
> > > > > the mbus stuff down their throat.
> > > 
> > > I disagree here, this forces us to use code to map the device tree enum
> > > to the kernel enum for Linux, i.e. adds complexity and maintenance work
> > > if additional bus_formats are needed.
> > > Assuming there is another OS which uses the device tree it would not
> > > make a difference, that OS would still need to map the device tree enum
> > > to the corresponding representation in their kernel.
> > 
> > So, you don't want to do something in Linux, but would expect someone
> > else to be completely ok with that?
> 
> Yes, sort of.
> Recycling the values as used currently in the Linux kernel rather than
> inventing a new numbering will make the Linux code a little easier to
> write, read and maintain without any negative effect on how that other
> OSs would have to map the DT representation to their internal representation.
> 
> Would you rather have something like:
> 
> <the common dt-bindings header file>
> DT_BUS_FMT_RGB666_1X18	1
> DT_BUS_FMT_RGB888_1X24	2
> ...
> 
> <panel-simple.c>
> switch (bus-format) {
> case DT_BUS_FMT_RGB666_1X18:
>   bus_format = MEDIA_BUS_FMT_RGB666_1X18; break;
> case DT_BUS_FMT_RGB888_1X24:
>   bus_format = MEDIA_BUS_FMT_RGB888_1X24; break;
> ...

I'm having a bit of trouble providing comments on this RFC, because I
don't believe I have a good enough overview of the different cases we
need to support (and in particular the corner cases). Max, you mentioned
an interesting one on Tegra platforms, would you maybe have a short (or
long, who knows) list of use cases you need to support now, or just know
about ? I think it would be easier to discuss the problem and the best
solution with concrete examples.

One particular thing that needs to be taken into account is that not all
devices (I'm talking about both the panel side and the source side) use
a data bus with contiguous bits. How to map a format to D[23:0] is one
thing, but there are devices that document pins as R[7:0], G[7:0],
B[7:0] (possibly with some permutations of the components). It's quite
easy to map between those two representations, once a mapping is
defined. I'd like these things to be considered explicitly instead of
relying on an implicit shared knowledge, as in my experience implicit
rules lead to one version per participant in the conversation :-)

> > > I would copy the definitions of media-bus-format.h into a header in
> > > include/dt-bindings similarly as it is done for
> > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > 
> > That might not be an option: that header is licensed under the GPL,
> > device trees are usually licensed under GPL+MIT, and we don't have any
> > requirements on the license for other projects using a DT (hence the
> > dual license).
> 
> That one I didn't consider. That would be solved by a newly invented
> enum.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Laurent Pinchart]

On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard wrote:
> > > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > > > On Fri, 18 Mar 2022 at 16:35, Maxime Ripard 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 wrote:
> > > > > > > > > On 3/2/22 15:21, Maxime Ripard wrote:
> > > > > > > > > > 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.
> > > 
> > > I agree that the strings used to set "data-mapping" weren't self explaining.
> > > However, as there was a
> > > clear 1:1 relation to the bus_format value the meaning
> > > wasn't ambiguous at all.
> > > 
> > > > > > 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.
> > > > > 
> > > > > Yeah, that's kind of my point actually :)
> > > > 
> > > > Ah, OK :)
> > > > 
> > > > > Just having a rgb666 string won't allow to differentiate between
> > > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > > > string but that usually leads to inconsistent or weird names, so this
> > > > > isn't ideal.
> > > 
> > > We're on the same page that the strings that were used aren't self
> > > explaining and do not follow a pattern which would make it easy to
> > > extend. However that is something I addressed in my RFC proposal, not?
> > > 
> > > > > > 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).
> > > > > 
> > > > > I think having an integer value is indeed better: it doesn't change much
> > > > > in the device tree if we're using a header, it makes the driver simpler
> > > > > since we don't have to parse a string, and we can easily extend it or
> > > > > rename the define, it won't change the ABI.
> > > 
> > > Fine with me.

I'm fine with integers too. Strings give the false impression that new
values can be added with a lower risk of a conflict, but that just a
false impression.

> > > > > I'm not sure using the raw media bus format value is ideal though, since
> > > > > that value could then be used by any OS, and it would effectively force
> > > > > the mbus stuff down their throat.
> > > 
> > > I disagree here, this forces us to use code to map the device tree enum
> > > to the kernel enum for Linux, i.e. adds complexity and maintenance work
> > > if additional bus_formats are needed.
> > > Assuming there is another OS which uses the device tree it would not
> > > make a difference, that OS would still need to map the device tree enum
> > > to the corresponding representation in their kernel.
> > 
> > So, you don't want to do something in Linux, but would expect someone
> > else to be completely ok with that?
> 
> Yes, sort of.
> Recycling the values as used currently in the Linux kernel rather than
> inventing a new numbering will make the Linux code a little easier to
> write, read and maintain without any negative effect on how that other
> OSs would have to map the DT representation to their internal representation.
> 
> Would you rather have something like:
> 
> <the common dt-bindings header file>
> DT_BUS_FMT_RGB666_1X18	1
> DT_BUS_FMT_RGB888_1X24	2
> ...
> 
> <panel-simple.c>
> switch (bus-format) {
> case DT_BUS_FMT_RGB666_1X18:
>   bus_format = MEDIA_BUS_FMT_RGB666_1X18; break;
> case DT_BUS_FMT_RGB888_1X24:
>   bus_format = MEDIA_BUS_FMT_RGB888_1X24; break;
> ...

I'm having a bit of trouble providing comments on this RFC, because I
don't believe I have a good enough overview of the different cases we
need to support (and in particular the corner cases). Max, you mentioned
an interesting one on Tegra platforms, would you maybe have a short (or
long, who knows) list of use cases you need to support now, or just know
about ? I think it would be easier to discuss the problem and the best
solution with concrete examples.

One particular thing that needs to be taken into account is that not all
devices (I'm talking about both the panel side and the source side) use
a data bus with contiguous bits. How to map a format to D[23:0] is one
thing, but there are devices that document pins as R[7:0], G[7:0],
B[7:0] (possibly with some permutations of the components). It's quite
easy to map between those two representations, once a mapping is
defined. I'd like these things to be considered explicitly instead of
relying on an implicit shared knowledge, as in my experience implicit
rules lead to one version per participant in the conversation :-)

> > > I would copy the definitions of media-bus-format.h into a header in
> > > include/dt-bindings similarly as it is done for
> > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > 
> > That might not be an option: that header is licensed under the GPL,
> > device trees are usually licensed under GPL+MIT, and we don't have any
> > requirements on the license for other projects using a DT (hence the
> > dual license).
> 
> That one I didn't consider. That would be solved by a newly invented
> enum.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Am Freitag, den 08.04.2022, 21:15 +0300 schrieb Laurent Pinchart:
> On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> > Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > > > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard wrote:
> > > > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > > > > On Fri, 18 Mar 2022 at 16:35, Maxime Ripard 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 wrote:
> > > > > > > > > > On 3/2/22 15:21, Maxime Ripard wrote:
> > > > > > > > > > > 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.
> > > > 
> > > > I agree that the strings used to set "data-mapping" weren't self explaining.
> > > > However, as there was a
> > > > clear 1:1 relation to the bus_format value the meaning
> > > > wasn't ambiguous at all.
> > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > Yeah, that's kind of my point actually :)
> > > > > 
> > > > > Ah, OK :)
> > > > > 
> > > > > > Just having a rgb666 string won't allow to differentiate between
> > > > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > > > > string but that usually leads to inconsistent or weird names, so this
> > > > > > isn't ideal.
> > > > 
> > > > We're on the same page that the strings that were used aren't self
> > > > explaining and do not follow a pattern which would make it easy to
> > > > extend. However that is something I addressed in my RFC proposal, not?
> > > > 
> > > > > > > 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).
> > > > > > 
> > > > > > I think having an integer value is indeed better: it doesn't change much
> > > > > > in the device tree if we're using a header, it makes the driver simpler
> > > > > > since we don't have to parse a string, and we can easily extend it or
> > > > > > rename the define, it won't change the ABI.
> > > > 
> > > > Fine with me.
> 
> I'm fine with integers too. Strings give the false impression that new
> values can be added with a lower risk of a conflict, but that just a
> false impression.
> 
> > > > > > I'm not sure using the raw media bus format value is ideal though, since
> > > > > > that value could then be used by any OS, and it would effectively force
> > > > > > the mbus stuff down their throat.
> > > > 
> > > > I disagree here, this forces us to use code to map the device tree enum
> > > > to the kernel enum for Linux, i.e. adds complexity and maintenance work
> > > > if additional bus_formats are needed.
> > > > Assuming there is another OS which uses the device tree it would not
> > > > make a difference, that OS would still need to map the device tree enum
> > > > to the corresponding representation in their kernel.
> > > 
> > > So, you don't want to do something in Linux, but would expect someone
> > > else to be completely ok with that?
> > 
> > Yes, sort of.
> > Recycling the values as used currently in the Linux kernel rather than
> > inventing a new numbering will make the Linux code a little easier to
> > write, read and maintain without any negative effect on how that other
> > OSs would have to map the DT representation to their internal representation.
> > 
> > Would you rather have something like:
> > 
> > <the common dt-bindings header file>
> > DT_BUS_FMT_RGB666_1X18	1
> > DT_BUS_FMT_RGB888_1X24	2
> > ...
> > 
> > <panel-simple.c>
> > switch (bus-format) {
> > case DT_BUS_FMT_RGB666_1X18:
> >   bus_format = MEDIA_BUS_FMT_RGB666_1X18; break;
> > case DT_BUS_FMT_RGB888_1X24:
> >   bus_format = MEDIA_BUS_FMT_RGB888_1X24; break;
> > ...
> 
> I'm having a bit of trouble providing comments on this RFC, because I
> don't believe I have a good enough overview of the different cases we
> need to support (and in particular the corner cases). Max, you mentioned
> an interesting one on Tegra platforms, would you maybe have a short (or
> long, who knows) list of use cases you need to support now, or just know
> about ? I think it would be easier to discuss the problem and the best
> solution with concrete examples.
> 
> One particular thing that needs to be taken into account is that not all
> devices (I'm talking about both the panel side and the source side) use
> a data bus with contiguous bits. How to map a format to D[23:0] is one
> thing, but there are devices that document pins as R[7:0], G[7:0],
> B[7:0] (possibly with some permutations of the components). It's quite
> easy to map between those two representations, once a mapping is
> defined. I'd like these things to be considered explicitly instead of
> relying on an implicit shared knowledge, as in my experience implicit
> rules lead to one version per participant in the conversation :-)


Panels:

Our customers only report panels with either 18 bit or 24 bit color
depth. So those panels have pins for R[7:0], G[7:0], B[7:0] or pins
for R[5:0], G[5:0], B[5:0].
By not connectiong LSBs of each color they can of course also be
connected to a controller which provides not all colors, e.g. a 24 bit
color panel can be connected to a 18 or 16 bit controller. The
same with a 18 bit color panel which can be connected to a 24 bit
controller by not connecting the LSBs of the controller.

Note that in the current panel-simple.c the common panels use one of:
- MEDIA_BUS_FMT_RGB565_1X16
- MEDIA_BUS_FMT_RGB666_1X18
- MEDIA_BUS_FMT_RGB888_1X24
Only two panel use an exotic bus format, giantplus_gpm940b0 uses three
bus cycles each 8 bit wide, MEDIA_BUS_FMT_RGB888_3X8.
The ti_nspire_classic_lcd_panel uses a 8 bit wide black and white
format MEDIA_BUS_FMT_Y8_1X8.


Controllers:

The data source provides usually up to 24 data lines with a
configurable assignment of the color info to these lines.

The controller side (and their Linux drivers) allow generally for
the following mappings (checked i.MX6 IPU, i.MX7 LCDIF, RPi vc4) with
the HW sometimes being more flexible but not reflected in SW:

23 22 21 20 19 18 17 16 15 14 13 12 11 10  9  8  7  6  5  4  3  2  1  0
MEDIA_BUS_FMT_RGB888_1X24
R7                   R0|G7                   G0|B7                   B0|
MEDIA_BUS_FMT_RGB666_1X18
N/A              |R5             R0|G5             G0|B5             B0|
MEDIA_BUS_FMT_RGB565_1X16
N/A                     |R4          R0|G5            G0|B4          B0|

They allow to swap the color from their default RGB to
RBG/GRB/GBR/BRG/BGR but the drivers usually only support RGB and maybe a
limited subset of the other combinations.

A stm32mp1 provides exclusively MEDIA_BUS_FMT_RGB888_1X24 without
any color swapping.

A Tegra30 provides additonally a format like this, i.e. a second RGB888
mapping, sort of like JEIDA vs. SPWG in 24bit LVDS:
R1 R0 G1 G0 B1 B0|R7             R2|G7             G2|B7             B2|

Unused lines can usually be used for something different through a
different pin configuration.

[1] documents the currently defined bus formats along with the mapping.


Connection:

The connection between the controller and the panel is usually done
with FFC flat cables or the likes. At best panels from one manufacturer
share a common pinout on that FFC cables if at all.
It's the HW designers job to create a pinout which fits the panel with
one of the available mappings the controller is able to provide.

The decision for which mapping to use depends for one on the panel
color depth of course.
For panels with less than 24 bits of color maybe the HW designer wants
alternate functions only available on certain pins being available.
Additionally using something other than the RGB order of the colors
(RBG, BGR ...) might ease the layout job.


(My) Conclusion:

It makes sense to describe the bus format as a enumeration. Whatever
fancy mapping a current or future controller might be able to provide
just add a sensible new enum element and use that in the controllers
driver. This is also what the Linux kernel today is using.

For the device tree, trying to break the possible mappings into
orthogonal structure elements (order RGB, BGR ...), color width
(888, 565 ...), alignment (Hi, Low ...) may not fit in some corner
case, the T30 one above a real world example.
Defining the full mapping (D0 -> B0, D1 -> B1, ...) seems to become
an unreadable data blob which in the Linux case will be additionally
a mess to translate into the Linux kernel bus format enum.

So my proposal still is to add a dts property which defines the used
bus format as an enum. Feedback already is to use a number together
with a header file giving a macro for each used bus format rather
than using strings.
I would use a new property name bus-format = <DT_MEDIA_BUS_FMT_RGB666_1X18>;
with a new binding header file with e.g.
```
#define DT_MEDIA_BUS_FMT_RGB565_1X16           0x1017
#define DT_MEDIA_BUS_FMT_RGB666_1X18           0x1009
#define DT_MEDIA_BUS_FMT_RBG888_1X24           0x100a
 ...
```
I have no strong opinion on using the Linux numbering for the enum
elements, so I would also be fine with starting from 0.

Max

[1] 
https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats

> 
> > > > I would copy the definitions of media-bus-format.h into a header in
> > > > include/dt-bindings similarly as it is done for
> > > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > > 
> > > That might not be an option: that header is licensed under the GPL,
> > > device trees are usually licensed under GPL+MIT, and we don't have any
> > > requirements on the license for other projects using a DT (hence the
> > > dual license).
> > 
> > That one I didn't consider. That would be solved by a newly invented
> > enum.

Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Max Krummenacher]

Am Freitag, den 08.04.2022, 21:15 +0300 schrieb Laurent Pinchart:
> On Wed, Mar 23, 2022 at 09:06:18PM +0100, Max Krummenacher wrote:
> > Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard:
> > > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote:
> > > > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson:
> > > > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard wrote:
> > > > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> > > > > > > On Fri, 18 Mar 2022 at 16:35, Maxime Ripard 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 wrote:
> > > > > > > > > > On 3/2/22 15:21, Maxime Ripard wrote:
> > > > > > > > > > > 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.
> > > > 
> > > > I agree that the strings used to set "data-mapping" weren't self explaining.
> > > > However, as there was a
> > > > clear 1:1 relation to the bus_format value the meaning
> > > > wasn't ambiguous at all.
> > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > Yeah, that's kind of my point actually :)
> > > > > 
> > > > > Ah, OK :)
> > > > > 
> > > > > > Just having a rgb666 string won't allow to differentiate between
> > > > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
> > > > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
> > > > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
> > > > > > string but that usually leads to inconsistent or weird names, so this
> > > > > > isn't ideal.
> > > > 
> > > > We're on the same page that the strings that were used aren't self
> > > > explaining and do not follow a pattern which would make it easy to
> > > > extend. However that is something I addressed in my RFC proposal, not?
> > > > 
> > > > > > > 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).
> > > > > > 
> > > > > > I think having an integer value is indeed better: it doesn't change much
> > > > > > in the device tree if we're using a header, it makes the driver simpler
> > > > > > since we don't have to parse a string, and we can easily extend it or
> > > > > > rename the define, it won't change the ABI.
> > > > 
> > > > Fine with me.
> 
> I'm fine with integers too. Strings give the false impression that new
> values can be added with a lower risk of a conflict, but that just a
> false impression.
> 
> > > > > > I'm not sure using the raw media bus format value is ideal though, since
> > > > > > that value could then be used by any OS, and it would effectively force
> > > > > > the mbus stuff down their throat.
> > > > 
> > > > I disagree here, this forces us to use code to map the device tree enum
> > > > to the kernel enum for Linux, i.e. adds complexity and maintenance work
> > > > if additional bus_formats are needed.
> > > > Assuming there is another OS which uses the device tree it would not
> > > > make a difference, that OS would still need to map the device tree enum
> > > > to the corresponding representation in their kernel.
> > > 
> > > So, you don't want to do something in Linux, but would expect someone
> > > else to be completely ok with that?
> > 
> > Yes, sort of.
> > Recycling the values as used currently in the Linux kernel rather than
> > inventing a new numbering will make the Linux code a little easier to
> > write, read and maintain without any negative effect on how that other
> > OSs would have to map the DT representation to their internal representation.
> > 
> > Would you rather have something like:
> > 
> > <the common dt-bindings header file>
> > DT_BUS_FMT_RGB666_1X18	1
> > DT_BUS_FMT_RGB888_1X24	2
> > ...
> > 
> > <panel-simple.c>
> > switch (bus-format) {
> > case DT_BUS_FMT_RGB666_1X18:
> >   bus_format = MEDIA_BUS_FMT_RGB666_1X18; break;
> > case DT_BUS_FMT_RGB888_1X24:
> >   bus_format = MEDIA_BUS_FMT_RGB888_1X24; break;
> > ...
> 
> I'm having a bit of trouble providing comments on this RFC, because I
> don't believe I have a good enough overview of the different cases we
> need to support (and in particular the corner cases). Max, you mentioned
> an interesting one on Tegra platforms, would you maybe have a short (or
> long, who knows) list of use cases you need to support now, or just know
> about ? I think it would be easier to discuss the problem and the best
> solution with concrete examples.
> 
> One particular thing that needs to be taken into account is that not all
> devices (I'm talking about both the panel side and the source side) use
> a data bus with contiguous bits. How to map a format to D[23:0] is one
> thing, but there are devices that document pins as R[7:0], G[7:0],
> B[7:0] (possibly with some permutations of the components). It's quite
> easy to map between those two representations, once a mapping is
> defined. I'd like these things to be considered explicitly instead of
> relying on an implicit shared knowledge, as in my experience implicit
> rules lead to one version per participant in the conversation :-)


Panels:

Our customers only report panels with either 18 bit or 24 bit color
depth. So those panels have pins for R[7:0], G[7:0], B[7:0] or pins
for R[5:0], G[5:0], B[5:0].
By not connectiong LSBs of each color they can of course also be
connected to a controller which provides not all colors, e.g. a 24 bit
color panel can be connected to a 18 or 16 bit controller. The
same with a 18 bit color panel which can be connected to a 24 bit
controller by not connecting the LSBs of the controller.

Note that in the current panel-simple.c the common panels use one of:
- MEDIA_BUS_FMT_RGB565_1X16
- MEDIA_BUS_FMT_RGB666_1X18
- MEDIA_BUS_FMT_RGB888_1X24
Only two panel use an exotic bus format, giantplus_gpm940b0 uses three
bus cycles each 8 bit wide, MEDIA_BUS_FMT_RGB888_3X8.
The ti_nspire_classic_lcd_panel uses a 8 bit wide black and white
format MEDIA_BUS_FMT_Y8_1X8.


Controllers:

The data source provides usually up to 24 data lines with a
configurable assignment of the color info to these lines.

The controller side (and their Linux drivers) allow generally for
the following mappings (checked i.MX6 IPU, i.MX7 LCDIF, RPi vc4) with
the HW sometimes being more flexible but not reflected in SW:

23 22 21 20 19 18 17 16 15 14 13 12 11 10  9  8  7  6  5  4  3  2  1  0
MEDIA_BUS_FMT_RGB888_1X24
R7                   R0|G7                   G0|B7                   B0|
MEDIA_BUS_FMT_RGB666_1X18
N/A              |R5             R0|G5             G0|B5             B0|
MEDIA_BUS_FMT_RGB565_1X16
N/A                     |R4          R0|G5            G0|B4          B0|

They allow to swap the color from their default RGB to
RBG/GRB/GBR/BRG/BGR but the drivers usually only support RGB and maybe a
limited subset of the other combinations.

A stm32mp1 provides exclusively MEDIA_BUS_FMT_RGB888_1X24 without
any color swapping.

A Tegra30 provides additonally a format like this, i.e. a second RGB888
mapping, sort of like JEIDA vs. SPWG in 24bit LVDS:
R1 R0 G1 G0 B1 B0|R7             R2|G7             G2|B7             B2|

Unused lines can usually be used for something different through a
different pin configuration.

[1] documents the currently defined bus formats along with the mapping.


Connection:

The connection between the controller and the panel is usually done
with FFC flat cables or the likes. At best panels from one manufacturer
share a common pinout on that FFC cables if at all.
It's the HW designers job to create a pinout which fits the panel with
one of the available mappings the controller is able to provide.

The decision for which mapping to use depends for one on the panel
color depth of course.
For panels with less than 24 bits of color maybe the HW designer wants
alternate functions only available on certain pins being available.
Additionally using something other than the RGB order of the colors
(RBG, BGR ...) might ease the layout job.


(My) Conclusion:

It makes sense to describe the bus format as a enumeration. Whatever
fancy mapping a current or future controller might be able to provide
just add a sensible new enum element and use that in the controllers
driver. This is also what the Linux kernel today is using.

For the device tree, trying to break the possible mappings into
orthogonal structure elements (order RGB, BGR ...), color width
(888, 565 ...), alignment (Hi, Low ...) may not fit in some corner
case, the T30 one above a real world example.
Defining the full mapping (D0 -> B0, D1 -> B1, ...) seems to become
an unreadable data blob which in the Linux case will be additionally
a mess to translate into the Linux kernel bus format enum.

So my proposal still is to add a dts property which defines the used
bus format as an enum. Feedback already is to use a number together
with a header file giving a macro for each used bus format rather
than using strings.
I would use a new property name bus-format = <DT_MEDIA_BUS_FMT_RGB666_1X18>;
with a new binding header file with e.g.
```
#define DT_MEDIA_BUS_FMT_RGB565_1X16           0x1017
#define DT_MEDIA_BUS_FMT_RGB666_1X18           0x1009
#define DT_MEDIA_BUS_FMT_RBG888_1X24           0x100a
 ...
```
I have no strong opinion on using the Linux numbering for the enum
elements, so I would also be fine with starting from 0.

Max

[1] 
https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats

> 
> > > > I would copy the definitions of media-bus-format.h into a header in
> > > > include/dt-bindings similarly as it is done for
> > > > include/dt-bindings/display/sdtv-standards.h for TV standards.
> > > 
> > > That might not be an option: that header is licensed under the GPL,
> > > device trees are usually licensed under GPL+MIT, and we don't have any
> > > requirements on the license for other projects using a DT (hence the
> > > dual license).
> > 
> > That one I didn't consider. That would be solved by a newly invented
> > enum.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel