[RESEND,v3,06/11] drm: add DT bindings documentation for atmel-hlcdc-dc driver

The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
controller device.

The HLCDC block provides a single RGB output port, and only supports LCD
panels connection to LCD panels for now.

The atmel,panel property link the HLCDC RGB output with the LCD panel
connected on this port (note that the HLCDC RGB connector implementation
makes use of the DRM panel framework).

Connection to other external devices (DRM bridges) might be added later by
mean of a new atmel,xxx (atmel,bridge) property.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt

Hi Boris,

Thank you for the patch.

On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote:
> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> controller device.
> 
> The HLCDC block provides a single RGB output port, and only supports LCD
> panels connection to LCD panels for now.
> 
> The atmel,panel property link the HLCDC RGB output with the LCD panel
> connected on this port (note that the HLCDC RGB connector implementation
> makes use of the DRM panel framework).
> 
> Connection to other external devices (DRM bridges) might be added later by
> mean of a new atmel,xxx (atmel,bridge) property.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> 
> diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode
> 100644
> index 0000000..594bdb2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> @@ -0,0 +1,59 @@
> +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver
> +
> +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> +
> +Required properties:
> + - compatible: value should be one of the following:
> +   "atmel,hlcdc-dc"
> + - interrupts: the HLCDC interrupt definition
> + - pinctrl-names: the pin control state names. Should contain "default",
> +   "rgb-444", "rgb-565", "rgb-666" and "rgb-888".
> + - pinctrl-[0-4]: should contain the pinctrl states described by pinctrl
> +   names.

Do you need to switch between the different pinctrl configurations at runtime, 
or is the configuration selected from the panel type, which doesn't change ?

> + - atmel,panel: Should contain a phandle with 2 parameters.
> +   The first cell is a phandle to a DRM panel device
> +   The second cell encodes the RGB mode, which can take the following
> values:
> +   * 0: RGB444
> +   * 1: RGB565
> +   * 2: RGB666
> +   * 3: RGB888
> +   The third cell encodes specific flags describing LCD signals
> configuration
> +   (see Atmel's datasheet for a full description of these
> fields):
> +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> +   * bit 4: DISPPOL: Display Signal Polarity
> +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup
> Configuration
> +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold
> Configuration
> +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time

If I'm not mistaken, those are HLCDC configuration values that depend on the 
panel type and characteristics. Shouldn't they then be queries from the panel 
through the drm_panel API at runtime instead of being specified in DT ? This 
would likely require extending the drm_panel API.

> +
> +Example:
> +
> +	hlcdc: hlcdc@f0030000 {
> +		compatible = "atmel,sama5d3-hlcdc";
> +		reg = <0xf0030000 0x2000>;
> +		clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>;
> +		clock-names = "periph_clk","sys_clk", "slow_clk";
> +		status = "disabled";
> +
> +		hlcdc-display-controller {
> +			compatible = "atmel,hlcdc-dc";
> +			interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>;
> +			pinctrl-names = "default", "rgb-444", "rgb-565", "rgb-666", 
"rgb-888";
> +			pinctrl-0 = <&pinctrl_lcd_base>;
> +			pinctrl-1 = <&pinctrl_lcd_base &pinctrl_lcd_rgb444>;
> +			pinctrl-2 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;
> +			pinctrl-3 = <&pinctrl_lcd_base &pinctrl_lcd_rgb666>;
> +			pinctrl-4 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>;
> +		};
> +
> +		hlcdc_pwm: hlcdc-pwm {
> +			compatible = "atmel,hlcdc-pwm";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_lcd_pwm>;
> +			#pwm-cells = <3>;
> +		};
> +	};

Hi Laurent,

On Thu, 10 Jul 2014 13:16:21 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> Thank you for the patch.
> 
> On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote:
> > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > controller device.
> > 
> > The HLCDC block provides a single RGB output port, and only supports LCD
> > panels connection to LCD panels for now.
> > 
> > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > connected on this port (note that the HLCDC RGB connector implementation
> > makes use of the DRM panel framework).
> > 
> > Connection to other external devices (DRM bridges) might be added later by
> > mean of a new atmel,xxx (atmel,bridge) property.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode
> > 100644
> > index 0000000..594bdb2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > @@ -0,0 +1,59 @@
> > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver
> > +
> > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > +
> > +Required properties:
> > + - compatible: value should be one of the following:
> > +   "atmel,hlcdc-dc"
> > + - interrupts: the HLCDC interrupt definition
> > + - pinctrl-names: the pin control state names. Should contain "default",
> > +   "rgb-444", "rgb-565", "rgb-666" and "rgb-888".
> > + - pinctrl-[0-4]: should contain the pinctrl states described by pinctrl
> > +   names.
> 
> Do you need to switch between the different pinctrl configurations at runtime, 
> or is the configuration selected from the panel type, which doesn't change ?

At the moment no, but if we ever need to support different devices on
the same RGB connector (actually Atmel's sama5d3xek boards have an
RGB to HDMI bridge connected on the same RGB connector) and these
devices do not support the same RGB mode (say your LCD panel supports
RGB888 and your RGB to HDMI bridge supports RGB555), then depending on
the output you select you'll have to change your pinctrl config at
runtime.

I'd say we could get rid of this runtime pinctrl config as a first step
if DT ABI stability was not required.
But it is, and I'd like to have a future proof binding to handle these
tricky cases when they occurs (if they ever do).

Anyway, I'm open to any other alternative that could let me add support
for this later on.

BTW, is there any reason for not defining an RGB connector type (I'm
currently defining HLCDC connector as an LVDS connector) ?

> 
> > + - atmel,panel: Should contain a phandle with 2 parameters.
> > +   The first cell is a phandle to a DRM panel device
> > +   The second cell encodes the RGB mode, which can take the following
> > values:
> > +   * 0: RGB444
> > +   * 1: RGB565
> > +   * 2: RGB666
> > +   * 3: RGB888
> > +   The third cell encodes specific flags describing LCD signals
> > configuration
> > +   (see Atmel's datasheet for a full description of these
> > fields):
> > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > +   * bit 4: DISPPOL: Display Signal Polarity
> > +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup
> > Configuration
> > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold
> > Configuration
> > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> 
> If I'm not mistaken, those are HLCDC configuration values that depend on the 
> panel type and characteristics. Shouldn't they then be queries from the panel 
> through the drm_panel API at runtime instead of being specified in DT ? This 
> would likely require extending the drm_panel API.

HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and
DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the
GUARDTIME value.

Another question I had regarding these flags is whether they were LCD
panel specific or a mix of panel and board implementation.
Take the VSYNC HSYNC polarity, of course the LCD panel defines what it
expects in terms of polarity, but nothing prevents the HW designer from
inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ?
A solution would be to override some drm_display_mode settings with
informations taken from the DT.

Thanks for your review.

Best Regards,

Boris

Hi Boris,

On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote:
> On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart wrote:
> > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote:
> > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > > controller device.
> > > 
> > > The HLCDC block provides a single RGB output port, and only supports LCD
> > > panels connection to LCD panels for now.
> > > 
> > > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > > connected on this port (note that the HLCDC RGB connector implementation
> > > makes use of the DRM panel framework).
> > > 
> > > Connection to other external devices (DRM bridges) might be added later
> > > by mean of a new atmel,xxx (atmel,bridge) property.
> > > 
> > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > ---
> > > 
> > >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode
> > > 100644
> > > index 0000000..594bdb2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > @@ -0,0 +1,59 @@
> > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver
> > > +
> > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD
> > > device.
> > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more
> > > details.
> > > +
> > > +Required properties:
> > > + - compatible: value should be one of the following:
> > > +   "atmel,hlcdc-dc"
> > > + - interrupts: the HLCDC interrupt definition
> > > + - pinctrl-names: the pin control state names. Should contain
> > > "default",
> > > +   "rgb-444", "rgb-565", "rgb-666" and "rgb-888".
> > > + - pinctrl-[0-4]: should contain the pinctrl states described by
> > > pinctrl
> > > +   names.
> > 
> > Do you need to switch between the different pinctrl configurations at
> > runtime, or is the configuration selected from the panel type, which
> > doesn't change ?
>
> At the moment no, but if we ever need to support different devices on
> the same RGB connector (actually Atmel's sama5d3xek boards have an
> RGB to HDMI bridge connected on the same RGB connector) and these
> devices do not support the same RGB mode (say your LCD panel supports
> RGB888 and your RGB to HDMI bridge supports RGB555), then depending on
> the output you select you'll have to change your pinctrl config at
> runtime.

Just to make sure I understand the use case correctly, are you talking about 
two devices (for example an RGB666 panel and an RGB888 RGB to HDMI bridge) 
connected to the same output, with the ability to switch between the two at 
runtime ? That's a valid case (on a side note we shouldn't forget that the 
option of using both devices at the same time should be supported as well), 
but I would probably go for a fixed pinctrl configuration that supports both, 
although switching configurations at runtime would be a micro-optimization 
that might make sense.

> I'd say we could get rid of this runtime pinctrl config as a first step
> if DT ABI stability was not required.
> But it is, and I'd like to have a future proof binding to handle these
> tricky cases when they occurs (if they ever do).

I think we have a shortcoming of the pinctrl API here in the general case. The 
API only allows you to select a single configuration per device. Imagine the 
same display controller, with two DPI outputs, each of them configurable in 
444, 565, 666 or 888 modes. With the current API we would have to create 4*4 = 
16 pinctrl configurations for all combinations. That obviously wouldn't scale, 
so we'll have to fix this eventually. From a DT stability point of view, I 
would thus avoid specifying multiple pinctrl configurations now until we come 
up with a standard way to support this use case.

> Anyway, I'm open to any other alternative that could let me add support
> for this later on.
> 
> BTW, is there any reason for not defining an RGB connector type (I'm
> currently defining HLCDC connector as an LVDS connector) ?

Not that I know of. The DRM API has been developed before display on embedded 
systems became such a hot topic. If we had to redo it today, panels might be 
exposed to userspace as such, with a connector. We have to live with the past, 
so the connector will stay, but adding a new RGB connector type could make 
sense (although we might need a different name, in a way the VGA and LVDS 
connectors also carry RGB signals).

> > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > +   The first cell is a phandle to a DRM panel device
> > > +   The second cell encodes the RGB mode, which can take the following
> > > values:
> > > +   * 0: RGB444
> > > +   * 1: RGB565
> > > +   * 2: RGB666
> > > +   * 3: RGB888
> > > +   The third cell encodes specific flags describing LCD signals
> > > configuration
> > > +   (see Atmel's datasheet for a full description of these
> > > fields):
> > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal
> > > Synchronization
> > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup
> > > Configuration
> > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold
> > > Configuration
> > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > 
> > If I'm not mistaken, those are HLCDC configuration values that depend on
> > the panel type and characteristics. Shouldn't they then be queries from
> > the panel through the drm_panel API at runtime instead of being specified
> > in DT ? This would likely require extending the drm_panel API.
> 
> HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and
> DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the
> GUARDTIME value.
> 
> Another question I had regarding these flags is whether they were LCD
> panel specific or a mix of panel and board implementation.
> Take the VSYNC HSYNC polarity, of course the LCD panel defines what it
> expects in terms of polarity, but nothing prevents the HW designer from
> inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ?
>
> A solution would be to override some drm_display_mode settings with
> informations taken from the DT.

Given that I gave the exact same argument during a V4L2 DT bindings design 
review, I can only agree :-) It thus makes sense to specify polarities in the 
HLCDC DT node. The RGB mode, however, should probably be queried from the 
panel, as I don't expect it to be board-dependent but only panel-dependent.

I'm not sure about the other bits in the third cell, maybe we should discuss 
them in more details. I'm always wary when I see DT bindings referring to a 
datasheet :-) Getting the information from the panel by default, with a 
possible override, is an interesting option. You would thus likely need 
several DT properties associated with each connection to a panel. Would it 
then make sense to use the OF graph DT bindings instead of the atmel,panel 
property to specify connections ? You could store per-connection data in the 
endpoint and/or port nodes.

> Thanks for your review.

You're welcome. Sorry for not having had time to review the driver itself. 
Given my limited bandwidth at the moment I've decided to concentrate on the DT 
bindings first.

On Fri, 11 Jul 2014 12:37:46 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote:
> > On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart wrote:
> > > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote:
> > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > > > controller device.
> > > > 
> > > > The HLCDC block provides a single RGB output port, and only supports LCD
> > > > panels connection to LCD panels for now.
> > > > 
> > > > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > > > connected on this port (note that the HLCDC RGB connector implementation
> > > > makes use of the DRM panel framework).
> > > > 
> > > > Connection to other external devices (DRM bridges) might be added later
> > > > by mean of a new atmel,xxx (atmel,bridge) property.
> > > > 
> > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++
> > > >  1 file changed, 59 insertions(+)
> > > >  create mode 100644
> > > >  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode
> > > > 100644
> > > > index 0000000..594bdb2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > @@ -0,0 +1,59 @@
> > > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver
> > > > +
> > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD
> > > > device.
> > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more
> > > > details.
> > > > +
> > > > +Required properties:
> > > > + - compatible: value should be one of the following:
> > > > +   "atmel,hlcdc-dc"
> > > > + - interrupts: the HLCDC interrupt definition
> > > > + - pinctrl-names: the pin control state names. Should contain
> > > > "default",
> > > > +   "rgb-444", "rgb-565", "rgb-666" and "rgb-888".
> > > > + - pinctrl-[0-4]: should contain the pinctrl states described by
> > > > pinctrl
> > > > +   names.
> > > 
> > > Do you need to switch between the different pinctrl configurations at
> > > runtime, or is the configuration selected from the panel type, which
> > > doesn't change ?
> >
> > At the moment no, but if we ever need to support different devices on
> > the same RGB connector (actually Atmel's sama5d3xek boards have an
> > RGB to HDMI bridge connected on the same RGB connector) and these
> > devices do not support the same RGB mode (say your LCD panel supports
> > RGB888 and your RGB to HDMI bridge supports RGB555), then depending on
> > the output you select you'll have to change your pinctrl config at
> > runtime.
> 
> Just to make sure I understand the use case correctly, are you talking about 
> two devices (for example an RGB666 panel and an RGB888 RGB to HDMI bridge) 
> connected to the same output, with the ability to switch between the two at 
> runtime ?

Exactly.

> That's a valid case (on a side note we shouldn't forget that the 
> option of using both devices at the same time should be supported as well), 

AFAICT this is only possible if both devices connected to the RGB
connector use the same mode.

> but I would probably go for a fixed pinctrl configuration that supports both, 
> although switching configurations at runtime would be a micro-optimization 
> that might make sense.

Yep, it should work, and I agree that we're unlikely to reuse some RGB
pins for other usage when the active device is the one using RGB666
mode.

> 
> > I'd say we could get rid of this runtime pinctrl config as a first step
> > if DT ABI stability was not required.
> > But it is, and I'd like to have a future proof binding to handle these
> > tricky cases when they occurs (if they ever do).
> 
> I think we have a shortcoming of the pinctrl API here in the general case. The 
> API only allows you to select a single configuration per device. Imagine the 
> same display controller, with two DPI outputs, each of them configurable in 
> 444, 565, 666 or 888 modes. With the current API we would have to create 4*4 = 
> 16 pinctrl configurations for all combinations. That obviously wouldn't scale, 
> so we'll have to fix this eventually. From a DT stability point of view, I 
> would thus avoid specifying multiple pinctrl configurations now until we come 
> up with a standard way to support this use case.

Given your inputs, I guess I'll drop dynamic pinctrl config for the
next version.

> 
> > Anyway, I'm open to any other alternative that could let me add support
> > for this later on.
> > 
> > BTW, is there any reason for not defining an RGB connector type (I'm
> > currently defining HLCDC connector as an LVDS connector) ?
> 
> Not that I know of. The DRM API has been developed before display on embedded 
> systems became such a hot topic. If we had to redo it today, panels might be 
> exposed to userspace as such, with a connector. We have to live with the past, 
> so the connector will stay, but adding a new RGB connector type could make 
> sense (although we might need a different name, in a way the VGA and LVDS 
> connectors also carry RGB signals).

I had the same concern: I didn't find how this kind of connectors
was named (most of the time they're just referenced as RGB) :-).
What about RAW_RGB ?

> 
> > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > +   The first cell is a phandle to a DRM panel device
> > > > +   The second cell encodes the RGB mode, which can take the following
> > > > values:
> > > > +   * 0: RGB444
> > > > +   * 1: RGB565
> > > > +   * 2: RGB666
> > > > +   * 3: RGB888
> > > > +   The third cell encodes specific flags describing LCD signals
> > > > configuration
> > > > +   (see Atmel's datasheet for a full description of these
> > > > fields):
> > > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal
> > > > Synchronization
> > > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup
> > > > Configuration
> > > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold
> > > > Configuration
> > > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > > 
> > > If I'm not mistaken, those are HLCDC configuration values that depend on
> > > the panel type and characteristics. Shouldn't they then be queries from
> > > the panel through the drm_panel API at runtime instead of being specified
> > > in DT ? This would likely require extending the drm_panel API.
> > 
> > HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and
> > DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the
> > GUARDTIME value.
> > 
> > Another question I had regarding these flags is whether they were LCD
> > panel specific or a mix of panel and board implementation.
> > Take the VSYNC HSYNC polarity, of course the LCD panel defines what it
> > expects in terms of polarity, but nothing prevents the HW designer from
> > inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ?
> >
> > A solution would be to override some drm_display_mode settings with
> > informations taken from the DT.
> 
> Given that I gave the exact same argument during a V4L2 DT bindings design 
> review, I can only agree :-) It thus makes sense to specify polarities in the 
> HLCDC DT node. The RGB mode, however, should probably be queried from the 
> panel, as I don't expect it to be board-dependent but only panel-dependent.

Yes, what I had in mind is some kind of RGB connector framework (or
just helper functions), where you could define the supported RGB modes
and rely on another infrastructure to define the supported drm display
modes.
Because RGB connector is not just related to panels: see this RGB to
HDMI bridge [1] (this is the one used on Atmel's dev boards).
Moreover, simple panels are connector agnostics for now, and I'm not
sure we want to change that.

[1]http://pdf1.alldatasheet.fr/datasheet-pdf/view/218068/SILICONIMAGE/SII9022.html

> 
> I'm not sure about the other bits in the third cell, maybe we should discuss 
> them in more details. I'm always wary when I see DT bindings referring to a 
> datasheet :-) Getting the information from the panel by default, with a 
> possible override, is an interesting option. You would thus likely need 
> several DT properties associated with each connection to a panel. Would it 
> then make sense to use the OF graph DT bindings instead of the atmel,panel 
> property to specify connections ? You could store per-connection data in the 
> endpoint and/or port nodes.

I'll take a look at these bindings and let you know if they match my
needs.

> 
> > Thanks for your review.
> 
> You're welcome. Sorry for not having had time to review the driver itself. 
> Given my limited bandwidth at the moment I've decided to concentrate on the DT 
> bindings first.

No problem, after all DT bindings is the most tricky part of our work
nowadays, isn't it ;-) ?

Thanks,

Boris

On Fri, 11 Jul 2014 14:00:25 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> On Fri, 11 Jul 2014 12:37:46 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> 
> > Hi Boris,
> > 
> > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote:
> > > On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart wrote:
> > > > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote:
> > > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > > > > controller device.
> > > > > 
> > > > > The HLCDC block provides a single RGB output port, and only supports LCD
> > > > > panels connection to LCD panels for now.
> > > > > 
> > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > > > > connected on this port (note that the HLCDC RGB connector implementation
> > > > > makes use of the DRM panel framework).
> > > > > 
> > > > > Connection to other external devices (DRM bridges) might be added later
> > > > > by mean of a new atmel,xxx (atmel,bridge) property.
> > > > > 
> > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > > 
> > > > >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++
> > > > >  1 file changed, 59 insertions(+)
> > > > >  create mode 100644
> > > > >  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode
> > > > > 100644
> > > > > index 0000000..594bdb2
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > > @@ -0,0 +1,59 @@
> > > > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver
> > > > > +
> > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD
> > > > > device.
> > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more
> > > > > details.
> > > > > +
> > > > > +Required properties:
> > > > > + - compatible: value should be one of the following:
> > > > > +   "atmel,hlcdc-dc"
> > > > > + - interrupts: the HLCDC interrupt definition
> > > > > + - pinctrl-names: the pin control state names. Should contain
> > > > > "default",
> > > > > +   "rgb-444", "rgb-565", "rgb-666" and "rgb-888".
> > > > > + - pinctrl-[0-4]: should contain the pinctrl states described by
> > > > > pinctrl
> > > > > +   names.
> > > > 
> > > > Do you need to switch between the different pinctrl configurations at
> > > > runtime, or is the configuration selected from the panel type, which
> > > > doesn't change ?
> > >
> > > At the moment no, but if we ever need to support different devices on
> > > the same RGB connector (actually Atmel's sama5d3xek boards have an
> > > RGB to HDMI bridge connected on the same RGB connector) and these
> > > devices do not support the same RGB mode (say your LCD panel supports
> > > RGB888 and your RGB to HDMI bridge supports RGB555), then depending on
> > > the output you select you'll have to change your pinctrl config at
> > > runtime.
> > 
> > Just to make sure I understand the use case correctly, are you talking about 
> > two devices (for example an RGB666 panel and an RGB888 RGB to HDMI bridge) 
> > connected to the same output, with the ability to switch between the two at 
> > runtime ?
> 
> Exactly.
> 
> > That's a valid case (on a side note we shouldn't forget that the 
> > option of using both devices at the same time should be supported as well), 
> 
> AFAICT this is only possible if both devices connected to the RGB
> connector use the same mode.
> 
> > but I would probably go for a fixed pinctrl configuration that supports both, 
> > although switching configurations at runtime would be a micro-optimization 
> > that might make sense.
> 
> Yep, it should work, and I agree that we're unlikely to reuse some RGB
> pins for other usage when the active device is the one using RGB666
> mode.
> 
> > 
> > > I'd say we could get rid of this runtime pinctrl config as a first step
> > > if DT ABI stability was not required.
> > > But it is, and I'd like to have a future proof binding to handle these
> > > tricky cases when they occurs (if they ever do).
> > 
> > I think we have a shortcoming of the pinctrl API here in the general case. The 
> > API only allows you to select a single configuration per device. Imagine the 
> > same display controller, with two DPI outputs, each of them configurable in 
> > 444, 565, 666 or 888 modes. With the current API we would have to create 4*4 = 
> > 16 pinctrl configurations for all combinations. That obviously wouldn't scale, 
> > so we'll have to fix this eventually. From a DT stability point of view, I 
> > would thus avoid specifying multiple pinctrl configurations now until we come 
> > up with a standard way to support this use case.
> 
> Given your inputs, I guess I'll drop dynamic pinctrl config for the
> next version.
> 
> > 
> > > Anyway, I'm open to any other alternative that could let me add support
> > > for this later on.
> > > 
> > > BTW, is there any reason for not defining an RGB connector type (I'm
> > > currently defining HLCDC connector as an LVDS connector) ?
> > 
> > Not that I know of. The DRM API has been developed before display on embedded 
> > systems became such a hot topic. If we had to redo it today, panels might be 
> > exposed to userspace as such, with a connector. We have to live with the past, 
> > so the connector will stay, but adding a new RGB connector type could make 
> > sense (although we might need a different name, in a way the VGA and LVDS 
> > connectors also carry RGB signals).
> 
> I had the same concern: I didn't find how this kind of connectors
> was named (most of the time they're just referenced as RGB) :-).
> What about RAW_RGB ?

Okay, actually there is a widely used name for this kind of connectors
and it's "Parallel RGB" (thanks Thomas ;-)).

> 
> > 
> > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > +   The first cell is a phandle to a DRM panel device
> > > > > +   The second cell encodes the RGB mode, which can take the following
> > > > > values:
> > > > > +   * 0: RGB444
> > > > > +   * 1: RGB565
> > > > > +   * 2: RGB666
> > > > > +   * 3: RGB888
> > > > > +   The third cell encodes specific flags describing LCD signals
> > > > > configuration
> > > > > +   (see Atmel's datasheet for a full description of these
> > > > > fields):
> > > > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal
> > > > > Synchronization
> > > > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup
> > > > > Configuration
> > > > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold
> > > > > Configuration
> > > > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > > > 
> > > > If I'm not mistaken, those are HLCDC configuration values that depend on
> > > > the panel type and characteristics. Shouldn't they then be queries from
> > > > the panel through the drm_panel API at runtime instead of being specified
> > > > in DT ? This would likely require extending the drm_panel API.
> > > 
> > > HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and
> > > DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the
> > > GUARDTIME value.
> > > 
> > > Another question I had regarding these flags is whether they were LCD
> > > panel specific or a mix of panel and board implementation.
> > > Take the VSYNC HSYNC polarity, of course the LCD panel defines what it
> > > expects in terms of polarity, but nothing prevents the HW designer from
> > > inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ?
> > >
> > > A solution would be to override some drm_display_mode settings with
> > > informations taken from the DT.
> > 
> > Given that I gave the exact same argument during a V4L2 DT bindings design 
> > review, I can only agree :-) It thus makes sense to specify polarities in the 
> > HLCDC DT node. The RGB mode, however, should probably be queried from the 
> > panel, as I don't expect it to be board-dependent but only panel-dependent.
> 
> Yes, what I had in mind is some kind of RGB connector framework (or
> just helper functions), where you could define the supported RGB modes
> and rely on another infrastructure to define the supported drm display
> modes.
> Because RGB connector is not just related to panels: see this RGB to
> HDMI bridge [1] (this is the one used on Atmel's dev boards).
> Moreover, simple panels are connector agnostics for now, and I'm not
> sure we want to change that.
> 
> [1]http://pdf1.alldatasheet.fr/datasheet-pdf/view/218068/SILICONIMAGE/SII9022.html
> 
> > 
> > I'm not sure about the other bits in the third cell, maybe we should discuss 
> > them in more details. I'm always wary when I see DT bindings referring to a 
> > datasheet :-) Getting the information from the panel by default, with a 
> > possible override, is an interesting option. You would thus likely need 
> > several DT properties associated with each connection to a panel. Would it 
> > then make sense to use the OF graph DT bindings instead of the atmel,panel 
> > property to specify connections ? You could store per-connection data in the 
> > endpoint and/or port nodes.
> 
> I'll take a look at these bindings and let you know if they match my
> needs.
> 
> > 
> > > Thanks for your review.
> > 
> > You're welcome. Sorry for not having had time to review the driver itself. 
> > Given my limited bandwidth at the moment I've decided to concentrate on the DT 
> > bindings first.
> 
> No problem, after all DT bindings is the most tricky part of our work
> nowadays, isn't it ;-) ?
> 
> Thanks,
> 
> Boris
>

On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> controller device.
> 
> The HLCDC block provides a single RGB output port, and only supports LCD
> panels connection to LCD panels for now.
> 
> The atmel,panel property link the HLCDC RGB output with the LCD panel
> connected on this port (note that the HLCDC RGB connector implementation
> makes use of the DRM panel framework).
> 
> Connection to other external devices (DRM bridges) might be added later by
> mean of a new atmel,xxx (atmel,bridge) property.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt

This is the wrong directory. Device tree bindings describe hardware, but
DRM is a Linux-specific framework. And yes, there are already files in
that directory, I know, but that doesn't make it any better.

I suggest either devicetree/bindings/gpu or devicetree/bindings/video.

> diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
[...]
> +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.

I think it's better to refer to these using relative filenames. When the
device tree bindings are moved out of the kernel tree, they may no
longer use the same hierarchy.

> +Required properties:
> + - compatible: value should be one of the following:
> +   "atmel,hlcdc-dc"

There's only one value, so perhaps: should be "atmel,hlcdc-dc".

> + - atmel,panel: Should contain a phandle with 2 parameters.
> +   The first cell is a phandle to a DRM panel device
> +   The second cell encodes the RGB mode, which can take the following values:
> +   * 0: RGB444
> +   * 1: RGB565
> +   * 2: RGB666
> +   * 3: RGB888

These are properties of the panel and should be obtained from the panel
directly rather than an additional cell in this specifier.

> +   The third cell encodes specific flags describing LCD signals configuration
> +   (see Atmel's datasheet for a full description of these fields):
> +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> +   * bit 4: DISPPOL: Display Signal Polarity
> +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration
> +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration
> +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time

Similarly for most of these: HSPOL and VSPOL seem to correspond to the
DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
vsync_end of the same structure.

As for the others, maybe if you could explain what exactly they are we
may be able to find a better fit.

Thierry

On Fri, Jul 11, 2014 at 02:00:25PM +0200, Boris BREZILLON wrote:
> On Fri, 11 Jul 2014 12:37:46 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote:
[...]
> > > BTW, is there any reason for not defining an RGB connector type (I'm
> > > currently defining HLCDC connector as an LVDS connector) ?
> > 
> > Not that I know of. The DRM API has been developed before display on embedded 
> > systems became such a hot topic. If we had to redo it today, panels might be 
> > exposed to userspace as such, with a connector. We have to live with the past, 
> > so the connector will stay, but adding a new RGB connector type could make 
> > sense (although we might need a different name, in a way the VGA and LVDS 
> > connectors also carry RGB signals).
> 
> I had the same concern: I didn't find how this kind of connectors
> was named (most of the time they're just referenced as RGB) :-).
> What about RAW_RGB ?

Are there even panels that take raw RGB as input? In all cases I've seen
(which admittedly may not be all that many) there's always a transparent
RGB/LVDS bridge, so the "connector" is in fact LVDS, not RGB, even if
the display controller outputs RGB directly.

Thierry

Hello Thierry,

On Mon, 14 Jul 2014 12:05:43 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > controller device.
> > 
> > The HLCDC block provides a single RGB output port, and only supports LCD
> > panels connection to LCD panels for now.
> > 
> > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > connected on this port (note that the HLCDC RGB connector implementation
> > makes use of the DRM panel framework).
> > 
> > Connection to other external devices (DRM bridges) might be added later by
> > mean of a new atmel,xxx (atmel,bridge) property.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> 
> This is the wrong directory. Device tree bindings describe hardware, but
> DRM is a Linux-specific framework. And yes, there are already files in
> that directory, I know, but that doesn't make it any better.
> 
> I suggest either devicetree/bindings/gpu or devicetree/bindings/video.

No problem, I'll move the documentation into devicetree/bindings/video
(the HLCDC does not provide any 3D rendering functionality and thus
I'm not sure moving the bindings documentation into
devicetree/bindings/gpu makes sense).

> 
> > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> [...]
> > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> 
> I think it's better to refer to these using relative filenames. When the
> device tree bindings are moved out of the kernel tree, they may no
> longer use the same hierarchy.

Sure.
By relative path you mean ../../mfd/atmel-hlcdc.txt or just
mfd/atmel-hlcdc.txt ?

> 
> > +Required properties:
> > + - compatible: value should be one of the following:
> > +   "atmel,hlcdc-dc"
> 
> There's only one value, so perhaps: should be "atmel,hlcdc-dc".

Yes, I'll fix that.

> 
> > + - atmel,panel: Should contain a phandle with 2 parameters.
> > +   The first cell is a phandle to a DRM panel device
> > +   The second cell encodes the RGB mode, which can take the following values:
> > +   * 0: RGB444
> > +   * 1: RGB565
> > +   * 2: RGB666
> > +   * 3: RGB888
> 
> These are properties of the panel and should be obtained from the panel
> directly rather than an additional cell in this specifier.

Okay.
What's the preferred way of doing this ?
What about defining an rgb-mode property in the panel node.

BTW, have you received this series [1] adding support for the LCD panel
I'm testing this driver with.

[1] https://lkml.org/lkml/2014/6/5/612

> 
> > +   The third cell encodes specific flags describing LCD signals configuration
> > +   (see Atmel's datasheet for a full description of these fields):
> > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > +   * bit 4: DISPPOL: Display Signal Polarity
> > +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration
> > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration
> > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> 
> Similarly for most of these: HSPOL and VSPOL seem to correspond to the
> DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
> VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
> vsync_end of the same structure.

I agree with HSPOL and VSPOL.

> 
> As for the others, maybe if you could explain what exactly they are we
> may be able to find a better fit.

Atmel datasheets include several timing diagrams [2] (chapter "32.6.17
Output Timing Generation" page 603), and I think you will get more
informations from these diagrams than if I try to explain what I
understood ;-).

Best Regards,

Boris

[1]https://lkml.org/lkml/2014/6/5/612
[2]http://www.atmel.com/Images/Atmel_11121_32-bit-Cortex-A5-Microcontroller_SAMA5D3_Datasheet.pdf

Hi Boris,

On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > > controller device.
> > > 
> > > The HLCDC block provides a single RGB output port, and only supports LCD
> > > panels connection to LCD panels for now.
> > > 
> > > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > > connected on this port (note that the HLCDC RGB connector implementation
> > > makes use of the DRM panel framework).
> > > 
> > > Connection to other external devices (DRM bridges) might be added later
> > > by mean of a new atmel,xxx (atmel,bridge) property.
> > > 
> > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > ---
> > > 
> > >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++
> > >  1 file changed, 59 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt

[snip]

> > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > +   The first cell is a phandle to a DRM panel device
> > > +   The second cell encodes the RGB mode, which can take the following
> > > values: +   * 0: RGB444
> > > +   * 1: RGB565
> > > +   * 2: RGB666
> > > +   * 3: RGB888
> > 
> > These are properties of the panel and should be obtained from the panel
> > directly rather than an additional cell in this specifier.
> 
> Okay.
> What's the preferred way of doing this ?
> What about defining an rgb-mode property in the panel node.

You could do that, but it won't help you much, as the HLCDC driver must not 
parse properties from the panel node. You should instead extend the drm_panel 
API with a function to retrieve panel properties. The HLCDC driver will then 
query the panel driver at runtime for the interface type. The panel driver 
will get the information from hardcoded data in the driver, from DT or 
possibly in some cases by querying the panel hardware directly.

> BTW, have you received this series [1] adding support for the LCD panel
> I'm testing this driver with.
> 
> [1] https://lkml.org/lkml/2014/6/5/612
> 
> > > +   The third cell encodes specific flags describing LCD signals
> > > configuration
> > > +   (see Atmel's datasheet for a full description of these fields):
> > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal
> > > Synchronization
> > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup
> > > Configuration
> > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold
> > > Configuration
> > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > 
> > Similarly for most of these: HSPOL and VSPOL seem to correspond to the
> > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
> > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
> > vsync_end of the same structure.
> 
> I agree with HSPOL and VSPOL.
> 
> > As for the others, maybe if you could explain what exactly they are we
> > may be able to find a better fit.
> 
> Atmel datasheets include several timing diagrams [2] (chapter "32.6.17
> Output Timing Generation" page 603), and I think you will get more
> informations from these diagrams than if I try to explain what I
> understood ;-).

The VSP* bits fine-tune the VSYNC pulse generation timings by specifying the 
relationship between the VSYNC edges and the HSYNC pulses at the beginning and 
end of the VSYNC pulse. It might make sense to turn that into standard DRM 
properties, but we'll need to research if and how other vendor offer similar 
features.

As for GUARDTIME, I would split it into its own property. What are the typical 
values you have seen being used in read systems ?

On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
[...]
> > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > [...]
> > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > 
> > I think it's better to refer to these using relative filenames. When the
> > device tree bindings are moved out of the kernel tree, they may no
> > longer use the same hierarchy.
> 
> Sure.
> By relative path you mean ../../mfd/atmel-hlcdc.txt or just
> mfd/atmel-hlcdc.txt ?

I think the former is more explicit.

> > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > +   The first cell is a phandle to a DRM panel device
> > > +   The second cell encodes the RGB mode, which can take the following values:
> > > +   * 0: RGB444
> > > +   * 1: RGB565
> > > +   * 2: RGB666
> > > +   * 3: RGB888
> > 
> > These are properties of the panel and should be obtained from the panel
> > directly rather than an additional cell in this specifier.
> 
> Okay.
> What's the preferred way of doing this ?
> What about defining an rgb-mode property in the panel node.

There's .bpc in struct drm_display_info, I suspect that it could be used
for this. Alternatively, maybe we could extend the list of color formats
that go into drm_display_info.color_formats? RGB444 is already covered.

Also, like Laurent said, this shouldn't go into the device tree, since
it's already implied by the panel's compatible value, so we'd be
duplicating information.

> BTW, have you received this series [1] adding support for the LCD panel
> I'm testing this driver with.
> 
> [1] https://lkml.org/lkml/2014/6/5/612

I don't think I've seen it in my inbox, let me check my archives.

> > > +   The third cell encodes specific flags describing LCD signals configuration
> > > +   (see Atmel's datasheet for a full description of these fields):
> > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration
> > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration
> > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > 
> > Similarly for most of these: HSPOL and VSPOL seem to correspond to the
> > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
> > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
> > vsync_end of the same structure.
> 
> I agree with HSPOL and VSPOL.
> 
> > 
> > As for the others, maybe if you could explain what exactly they are we
> > may be able to find a better fit.
> 
> Atmel datasheets include several timing diagrams [2] (chapter "32.6.17
> Output Timing Generation" page 603), and I think you will get more
> informations from these diagrams than if I try to explain what I
> understood ;-).

These look like knobs to tune the signal in a very fine-grained manner.
To be honest, maybe the best way to solve this would be by omitting them
for now and choose some default that's likely to work on most devices.
Does the panel that you use specify how it expects HSYNC to be timed vs.
VSYNC?

Thierry

On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> Hi Boris,
> 
> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e.
> > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display
> > > > controller device.
> > > > 
> > > > The HLCDC block provides a single RGB output port, and only supports LCD
> > > > panels connection to LCD panels for now.
> > > > 
> > > > The atmel,panel property link the HLCDC RGB output with the LCD panel
> > > > connected on this port (note that the HLCDC RGB connector implementation
> > > > makes use of the DRM panel framework).
> > > > 
> > > > Connection to other external devices (DRM bridges) might be added later
> > > > by mean of a new atmel,xxx (atmel,bridge) property.
> > > > 
> > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++++++
> > > >  1 file changed, 59 insertions(+)
> > > >  create mode 100644
> > > >  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> 
> [snip]
> 
> > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > +   The first cell is a phandle to a DRM panel device
> > > > +   The second cell encodes the RGB mode, which can take the following
> > > > values: +   * 0: RGB444
> > > > +   * 1: RGB565
> > > > +   * 2: RGB666
> > > > +   * 3: RGB888
> > > 
> > > These are properties of the panel and should be obtained from the panel
> > > directly rather than an additional cell in this specifier.
> > 
> > Okay.
> > What's the preferred way of doing this ?
> > What about defining an rgb-mode property in the panel node.
> 
> You could do that, but it won't help you much, as the HLCDC driver must not 
> parse properties from the panel node. You should instead extend the drm_panel 
> API with a function to retrieve panel properties. The HLCDC driver will then 
> query the panel driver at runtime for the interface type. The panel driver 
> will get the information from hardcoded data in the driver, from DT or 
> possibly in some cases by querying the panel hardware directly.

My preference for this would be that we either add this to some existing
structure (struct drm_display_info seems like a good candidate) or if
the number of parameters grows out of hands, then maybe even introduce a
new type of device that's specific for the interface. DRM panels are an
abstraction for panels, that is, interface-agnostic, and if we start
exposing interface specific parameters things will start to become very
unwieldy.

Thierry

Hi Thierry,

On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> >>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> >>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> >>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a
> >>>> display controller device.
> >>>> 
> >>>> The HLCDC block provides a single RGB output port, and only supports
> >>>> LCD panels connection to LCD panels for now.
> >>>> 
> >>>> The atmel,panel property link the HLCDC RGB output with the LCD
> >>>> panel connected on this port (note that the HLCDC RGB connector
> >>>> implementation makes use of the DRM panel framework).
> >>>> 
> >>>> Connection to other external devices (DRM bridges) might be added
> >>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> >>>> 
> >>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> >>>> ---
> >>>> 
> >>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++
> >>>>  1 file changed, 59 insertions(+)
> >>>>  create mode 100644
> >>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > 
> > [snip]
> > 
> >>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> >>>> +   The first cell is a phandle to a DRM panel device
> >>>> +   The second cell encodes the RGB mode, which can take the
> >>>> following values:
> >>>> +   * 0: RGB444
> >>>> +   * 1: RGB565
> >>>> +   * 2: RGB666
> >>>> +   * 3: RGB888
> >>> 
> >>> These are properties of the panel and should be obtained from the
> >>> panel directly rather than an additional cell in this specifier.
> >> 
> >> Okay.
> >> What's the preferred way of doing this ?
> >> What about defining an rgb-mode property in the panel node.
> > 
> > You could do that, but it won't help you much, as the HLCDC driver must
> > not parse properties from the panel node. You should instead extend the
> > drm_panel API with a function to retrieve panel properties. The HLCDC
> > driver will then query the panel driver at runtime for the interface
> > type. The panel driver will get the information from hardcoded data in
> > the driver, from DT or possibly in some cases by querying the panel
> > hardware directly.
> 
> My preference for this would be that we either add this to some existing
> structure (struct drm_display_info seems like a good candidate) or if
> the number of parameters grows out of hands, then maybe even introduce a
> new type of device that's specific for the interface. DRM panels are an
> abstraction for panels, that is, interface-agnostic, and if we start
> exposing interface specific parameters things will start to become very
> unwieldy.

I agree with the goal of keeping drm_panel interface-agnostic. However, one 
way or another, interface parameters will need to be communicated between the 
panel driver and the controller driver. My preference, if we need to extend 
the number and/or scope of parameters beyond what drm_display_info could 
reasonably contain, would be to implement a new drm_panel operation to 
query/configure interface parameters, using a structure that contains the 
interface type and a union of type-specific structures. This would keep the 
API generic in the sense of not requiring explicit knowledge of all interfaces 
in the drivers, while offering the flexibility we need with a way to easily 
detect the interface type at runtime and react on unknown/unsupported types.

On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> > On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> > >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > >>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > >>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> > >>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a
> > >>>> display controller device.
> > >>>> 
> > >>>> The HLCDC block provides a single RGB output port, and only supports
> > >>>> LCD panels connection to LCD panels for now.
> > >>>> 
> > >>>> The atmel,panel property link the HLCDC RGB output with the LCD
> > >>>> panel connected on this port (note that the HLCDC RGB connector
> > >>>> implementation makes use of the DRM panel framework).
> > >>>> 
> > >>>> Connection to other external devices (DRM bridges) might be added
> > >>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> > >>>> 
> > >>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > >>>> ---
> > >>>> 
> > >>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++
> > >>>>  1 file changed, 59 insertions(+)
> > >>>>  create mode 100644
> > >>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > 
> > > [snip]
> > > 
> > >>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > >>>> +   The first cell is a phandle to a DRM panel device
> > >>>> +   The second cell encodes the RGB mode, which can take the
> > >>>> following values:
> > >>>> +   * 0: RGB444
> > >>>> +   * 1: RGB565
> > >>>> +   * 2: RGB666
> > >>>> +   * 3: RGB888
> > >>> 
> > >>> These are properties of the panel and should be obtained from the
> > >>> panel directly rather than an additional cell in this specifier.
> > >> 
> > >> Okay.
> > >> What's the preferred way of doing this ?
> > >> What about defining an rgb-mode property in the panel node.
> > > 
> > > You could do that, but it won't help you much, as the HLCDC driver must
> > > not parse properties from the panel node. You should instead extend the
> > > drm_panel API with a function to retrieve panel properties. The HLCDC
> > > driver will then query the panel driver at runtime for the interface
> > > type. The panel driver will get the information from hardcoded data in
> > > the driver, from DT or possibly in some cases by querying the panel
> > > hardware directly.
> > 
> > My preference for this would be that we either add this to some existing
> > structure (struct drm_display_info seems like a good candidate) or if
> > the number of parameters grows out of hands, then maybe even introduce a
> > new type of device that's specific for the interface. DRM panels are an
> > abstraction for panels, that is, interface-agnostic, and if we start
> > exposing interface specific parameters things will start to become very
> > unwieldy.
> 
> I agree with the goal of keeping drm_panel interface-agnostic. However, one 
> way or another, interface parameters will need to be communicated between the 
> panel driver and the controller driver. My preference, if we need to extend 
> the number and/or scope of parameters beyond what drm_display_info could 
> reasonably contain, would be to implement a new drm_panel operation to 
> query/configure interface parameters, using a structure that contains the 
> interface type and a union of type-specific structures. This would keep the 
> API generic in the sense of not requiring explicit knowledge of all interfaces 
> in the drivers, while offering the flexibility we need with a way to easily 
> detect the interface type at runtime and react on unknown/unsupported types.

That's exactly what I was hoping could be avoided. If instead we modeled
the interface type as a bus, we could for example have an lvds_bus along
with an lvds_device and then use that as the natural place to store
these properties. Much like we do for DSI.

Thierry

Hi Thierry,

On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote:
> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> >> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> >>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> >>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> >>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> >>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> >>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a
> >>>>>> display controller device.
> >>>>>> 
> >>>>>> The HLCDC block provides a single RGB output port, and only
> >>>>>> supports LCD panels connection to LCD panels for now.
> >>>>>> 
> >>>>>> The atmel,panel property link the HLCDC RGB output with the LCD
> >>>>>> panel connected on this port (note that the HLCDC RGB connector
> >>>>>> implementation makes use of the DRM panel framework).
> >>>>>> 
> >>>>>> Connection to other external devices (DRM bridges) might be added
> >>>>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> >>>>>> 
> >>>>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++++
> >>>>>>  1 file changed, 59 insertions(+)
> >>>>>>  create mode 100644
> >>>>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> >>> 
> >>> [snip]
> >>> 
> >>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> >>>>>> +   The first cell is a phandle to a DRM panel device
> >>>>>> +   The second cell encodes the RGB mode, which can take the
> >>>>>> following values:
> >>>>>> +   * 0: RGB444
> >>>>>> +   * 1: RGB565
> >>>>>> +   * 2: RGB666
> >>>>>> +   * 3: RGB888
> >>>>> 
> >>>>> These are properties of the panel and should be obtained from the
> >>>> panel directly rather than an additional cell in this specifier.
> >>>> 
> >>>> Okay.
> >>>> What's the preferred way of doing this ?
> >>>> What about defining an rgb-mode property in the panel node.
> >>> 
> >>> You could do that, but it won't help you much, as the HLCDC driver
> >>> must not parse properties from the panel node. You should instead
> >>> extend the drm_panel API with a function to retrieve panel properties.
> >>> The HLCDC driver will then query the panel driver at runtime for the
> >>> interface type. The panel driver will get the information from
> >>> hardcoded data in the driver, from DT or possibly in some cases by
> >>> querying the panel hardware directly.
> >> 
> >> My preference for this would be that we either add this to some existing
> >> structure (struct drm_display_info seems like a good candidate) or if
> >> the number of parameters grows out of hands, then maybe even introduce a
> >> new type of device that's specific for the interface. DRM panels are an
> >> abstraction for panels, that is, interface-agnostic, and if we start
> >> exposing interface specific parameters things will start to become very
> >> unwieldy.
> > 
> > I agree with the goal of keeping drm_panel interface-agnostic. However,
> > one way or another, interface parameters will need to be communicated
> > between the panel driver and the controller driver. My preference, if we
> > need to extend the number and/or scope of parameters beyond what
> > drm_display_info could reasonably contain, would be to implement a new
> > drm_panel operation to query/configure interface parameters, using a
> > structure that contains the interface type and a union of type-specific
> > structures. This would keep the API generic in the sense of not requiring
> > explicit knowledge of all interfaces in the drivers, while offering the
> > flexibility we need with a way to easily detect the interface type at
> > runtime and react on unknown/unsupported types.
>
> That's exactly what I was hoping could be avoided. If instead we modeled
> the interface type as a bus, we could for example have an lvds_bus along
> with an lvds_device and then use that as the natural place to store
> these properties. Much like we do for DSI.

And I believe that's what we should avoid ;-) First of all, let's not forget 
that Linux models control busses, not data busses. DSI is a special case as it 
combines the control and data busses, but in the general case the same 
implementation isn't possible. An LVDS panel controlled through I2C needs to 
be an I2C device sitting on an I2C bus.

Then, I believe it would make all drivers simpler if we had a single object 
type to deal with, with proper abstractions for bus types. A drm_panel that 
can model panels regardless of the data bus type, with one operation that 
conveys bus-specific information, makes storing the objects and communicating 
with them simpler than having to deal with different kind of devices.

On Mon, 14 Jul 2014 12:18:08 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Fri, Jul 11, 2014 at 02:00:25PM +0200, Boris BREZILLON wrote:
> > On Fri, 11 Jul 2014 12:37:46 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote:
> [...]
> > > > BTW, is there any reason for not defining an RGB connector type (I'm
> > > > currently defining HLCDC connector as an LVDS connector) ?
> > > 
> > > Not that I know of. The DRM API has been developed before display on embedded 
> > > systems became such a hot topic. If we had to redo it today, panels might be 
> > > exposed to userspace as such, with a connector. We have to live with the past, 
> > > so the connector will stay, but adding a new RGB connector type could make 
> > > sense (although we might need a different name, in a way the VGA and LVDS 
> > > connectors also carry RGB signals).
> > 
> > I had the same concern: I didn't find how this kind of connectors
> > was named (most of the time they're just referenced as RGB) :-).
> > What about RAW_RGB ?
> 
> Are there even panels that take raw RGB as input? In all cases I've seen
> (which admittedly may not be all that many) there's always a transparent
> RGB/LVDS bridge, so the "connector" is in fact LVDS, not RGB, even if
> the display controller outputs RGB directly.

At least the LCD module I'm using (FL500WVR00-A0T) is taking raw RGB as
input ;-).

Best Regards,

Boris

On Tue, 15 Jul 2014 12:52:54 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> > > On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> > > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> > > >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > > >>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > > >>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> > > >>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a
> > > >>>> display controller device.
> > > >>>> 
> > > >>>> The HLCDC block provides a single RGB output port, and only supports
> > > >>>> LCD panels connection to LCD panels for now.
> > > >>>> 
> > > >>>> The atmel,panel property link the HLCDC RGB output with the LCD
> > > >>>> panel connected on this port (note that the HLCDC RGB connector
> > > >>>> implementation makes use of the DRM panel framework).
> > > >>>> 
> > > >>>> Connection to other external devices (DRM bridges) might be added
> > > >>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> > > >>>> 
> > > >>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > >>>> ---
> > > >>>> 
> > > >>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 +++++++++++
> > > >>>>  1 file changed, 59 insertions(+)
> > > >>>>  create mode 100644
> > > >>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > 
> > > > [snip]
> > > > 
> > > >>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > > >>>> +   The first cell is a phandle to a DRM panel device
> > > >>>> +   The second cell encodes the RGB mode, which can take the
> > > >>>> following values:
> > > >>>> +   * 0: RGB444
> > > >>>> +   * 1: RGB565
> > > >>>> +   * 2: RGB666
> > > >>>> +   * 3: RGB888
> > > >>> 
> > > >>> These are properties of the panel and should be obtained from the
> > > >>> panel directly rather than an additional cell in this specifier.
> > > >> 
> > > >> Okay.
> > > >> What's the preferred way of doing this ?
> > > >> What about defining an rgb-mode property in the panel node.
> > > > 
> > > > You could do that, but it won't help you much, as the HLCDC driver must
> > > > not parse properties from the panel node. You should instead extend the
> > > > drm_panel API with a function to retrieve panel properties. The HLCDC
> > > > driver will then query the panel driver at runtime for the interface
> > > > type. The panel driver will get the information from hardcoded data in
> > > > the driver, from DT or possibly in some cases by querying the panel
> > > > hardware directly.
> > > 
> > > My preference for this would be that we either add this to some existing
> > > structure (struct drm_display_info seems like a good candidate) or if
> > > the number of parameters grows out of hands, then maybe even introduce a
> > > new type of device that's specific for the interface. DRM panels are an
> > > abstraction for panels, that is, interface-agnostic, and if we start
> > > exposing interface specific parameters things will start to become very
> > > unwieldy.
> > 
> > I agree with the goal of keeping drm_panel interface-agnostic. However, one 
> > way or another, interface parameters will need to be communicated between the 
> > panel driver and the controller driver. My preference, if we need to extend 
> > the number and/or scope of parameters beyond what drm_display_info could 
> > reasonably contain, would be to implement a new drm_panel operation to 
> > query/configure interface parameters, using a structure that contains the 
> > interface type and a union of type-specific structures. This would keep the 
> > API generic in the sense of not requiring explicit knowledge of all interfaces 
> > in the drivers, while offering the flexibility we need with a way to easily 
> > detect the interface type at runtime and react on unknown/unsupported types.
> 
> That's exactly what I was hoping could be avoided. If instead we modeled
> the interface type as a bus, we could for example have an lvds_bus along
> with an lvds_device and then use that as the natural place to store
> these properties. Much like we do for DSI.

I understand this is not a simple case here, and this is why I left
RGB mode config in the HLCDC node in the first place.

Anyway, I agree that this rgb mode should not be defined in the hlcdc
node but rather in the slave device. I said slave device and not panel
device here because the device connected to the RGB connector is not
necessarily an LCD panel (i.e. atmel is connecting a raw RGB to HDMI
bridge on the RGB connector). And given that I definitely think an
interface bus architecture is better: this way we could configure RGB
mode no matter what kind of device is connected on this bus and we
could keep slave devices interface-agnostic.

This being said, I guess modeling interface (or connector) types as
busses is not that simple.

I really want to help here, so let me know what I can do.

Just a side note: you are saying that RGB mode is a panel property, and
this is not entirely true (it might depends on board design) :-). In
some HW designs, LSB bits of the RGB connector are either connected to
ground or to the first available MSB bit. This way you can use an LCD
panel supporting RGB888 mode with an display controller supporting lower
modes (RGB555 or RGB666).

Best Regards,

Boris

Hi Laurent,

On Tue, 15 Jul 2014 13:07:58 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Thierry,
> 
> On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote:
> > On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> > > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> > >> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> > >>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> > >>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > >>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > >>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> > >>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a
> > >>>>>> display controller device.
> > >>>>>> 
> > >>>>>> The HLCDC block provides a single RGB output port, and only
> > >>>>>> supports LCD panels connection to LCD panels for now.
> > >>>>>> 
> > >>>>>> The atmel,panel property link the HLCDC RGB output with the LCD
> > >>>>>> panel connected on this port (note that the HLCDC RGB connector
> > >>>>>> implementation makes use of the DRM panel framework).
> > >>>>>> 
> > >>>>>> Connection to other external devices (DRM bridges) might be added
> > >>>>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> > >>>>>> 
> > >>>>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > >>>>>> ---
> > >>>>>> 
> > >>>>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++++
> > >>>>>>  1 file changed, 59 insertions(+)
> > >>>>>>  create mode 100644
> > >>>>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > >>> 
> > >>> [snip]
> > >>> 
> > >>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > >>>>>> +   The first cell is a phandle to a DRM panel device
> > >>>>>> +   The second cell encodes the RGB mode, which can take the
> > >>>>>> following values:
> > >>>>>> +   * 0: RGB444
> > >>>>>> +   * 1: RGB565
> > >>>>>> +   * 2: RGB666
> > >>>>>> +   * 3: RGB888
> > >>>>> 
> > >>>>> These are properties of the panel and should be obtained from the
> > >>>> panel directly rather than an additional cell in this specifier.
> > >>>> 
> > >>>> Okay.
> > >>>> What's the preferred way of doing this ?
> > >>>> What about defining an rgb-mode property in the panel node.
> > >>> 
> > >>> You could do that, but it won't help you much, as the HLCDC driver
> > >>> must not parse properties from the panel node. You should instead
> > >>> extend the drm_panel API with a function to retrieve panel properties.
> > >>> The HLCDC driver will then query the panel driver at runtime for the
> > >>> interface type. The panel driver will get the information from
> > >>> hardcoded data in the driver, from DT or possibly in some cases by
> > >>> querying the panel hardware directly.
> > >> 
> > >> My preference for this would be that we either add this to some existing
> > >> structure (struct drm_display_info seems like a good candidate) or if
> > >> the number of parameters grows out of hands, then maybe even introduce a
> > >> new type of device that's specific for the interface. DRM panels are an
> > >> abstraction for panels, that is, interface-agnostic, and if we start
> > >> exposing interface specific parameters things will start to become very
> > >> unwieldy.
> > > 
> > > I agree with the goal of keeping drm_panel interface-agnostic. However,
> > > one way or another, interface parameters will need to be communicated
> > > between the panel driver and the controller driver. My preference, if we
> > > need to extend the number and/or scope of parameters beyond what
> > > drm_display_info could reasonably contain, would be to implement a new
> > > drm_panel operation to query/configure interface parameters, using a
> > > structure that contains the interface type and a union of type-specific
> > > structures. This would keep the API generic in the sense of not requiring
> > > explicit knowledge of all interfaces in the drivers, while offering the
> > > flexibility we need with a way to easily detect the interface type at
> > > runtime and react on unknown/unsupported types.
> >
> > That's exactly what I was hoping could be avoided. If instead we modeled
> > the interface type as a bus, we could for example have an lvds_bus along
> > with an lvds_device and then use that as the natural place to store
> > these properties. Much like we do for DSI.
> 
> And I believe that's what we should avoid ;-) First of all, let's not forget 
> that Linux models control busses, not data busses. DSI is a special case as it 
> combines the control and data busses, but in the general case the same 
> implementation isn't possible. An LVDS panel controlled through I2C needs to 
> be an I2C device sitting on an I2C bus.
> 
> Then, I believe it would make all drivers simpler if we had a single object 
> type to deal with, with proper abstractions for bus types. A drm_panel that 
> can model panels regardless of the data bus type, with one operation that 
> conveys bus-specific information, makes storing the objects and communicating 
> with them simpler than having to deal with different kind of devices.
> 

Could you detail a bit what you mean by "single object type" ?

Is this about making a common abstraction class (by mean of
drm_xxx and drm_xxx_funcs) that could represent any display device
(drm_bridge, drm_panel, ...) ?

Best Regards,

Boris

Hi Boris,

On Wednesday 16 July 2014 15:05:22 Boris BREZILLON wrote:
> On Tue, 15 Jul 2014 13:07:58 +0200 Laurent Pinchart wrote:
> > On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote:
> >> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> >>> On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> >>>> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> >>>>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> >>>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> >>>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> >>>>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> >>>>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides
> >>>>>>>> a display controller device.
> >>>>>>>> 
> >>>>>>>> The HLCDC block provides a single RGB output port, and only
> >>>>>>>> supports LCD panels connection to LCD panels for now.
> >>>>>>>> 
> >>>>>>>> The atmel,panel property link the HLCDC RGB output with the LCD
> >>>>>>>> panel connected on this port (note that the HLCDC RGB connector
> >>>>>>>> implementation makes use of the DRM panel framework).
> >>>>>>>> 
> >>>>>>>> Connection to other external devices (DRM bridges) might be added
> >>>>>>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Boris BREZILLON
> >>>>>>>> <boris.brezillon@free-electrons.com>
> >>>>>>>> ---
> >>>>>>>> 
> >>>>>>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++
> >>>>>>>>  1 file changed, 59 insertions(+)
> >>>>>>>>  create mode 100644
> >>>>>>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> >>>>> 
> >>>>> [snip]
> >>>>> 
> >>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> >>>>>>>> +   The first cell is a phandle to a DRM panel device
> >>>>>>>> +   The second cell encodes the RGB mode, which can take the
> >>>>>>>> following values:
> >>>>>>>> +   * 0: RGB444
> >>>>>>>> +   * 1: RGB565
> >>>>>>>> +   * 2: RGB666
> >>>>>>>> +   * 3: RGB888
> >>>>>>> 
> >>>>>>> These are properties of the panel and should be obtained from the
> >>>>>>> panel directly rather than an additional cell in this specifier.
> >>>>>> 
> >>>>>> Okay.
> >>>>>> What's the preferred way of doing this ?
> >>>>>> What about defining an rgb-mode property in the panel node.
> >>>>> 
> >>>>> You could do that, but it won't help you much, as the HLCDC driver
> >>>>> must not parse properties from the panel node. You should instead
> >>>>> extend the drm_panel API with a function to retrieve panel
> >>>>> properties. The HLCDC driver will then query the panel driver at
> >>>>> runtime for the interface type. The panel driver will get the
> >>>>> information from hardcoded data in the driver, from DT or possibly
> >>>>> in some cases by querying the panel hardware directly.
> >>>> 
> >>>> My preference for this would be that we either add this to some
> >>>> existing structure (struct drm_display_info seems like a good
> >>>> candidate) or if the number of parameters grows out of hands, then
> >>>> maybe even introduce a new type of device that's specific for the
> >>>> interface. DRM panels are an abstraction for panels, that is,
> >>>> interface-agnostic, and if we start exposing interface specific
> >>>> parameters things will start to become very unwieldy.
> >>> 
> >>> I agree with the goal of keeping drm_panel interface-agnostic.
> >>> However, one way or another, interface parameters will need to be
> >>> communicated between the panel driver and the controller driver. My
> >>> preference, if we need to extend the number and/or scope of parameters
> >>> beyond what drm_display_info could reasonably contain, would be to
> >>> implement a new drm_panel operation to query/configure interface
> >>> parameters, using a structure that contains the interface type and a
> >>> union of type-specific structures. This would keep the API generic in
> >>> the sense of not requiring explicit knowledge of all interfaces in the
> >>> drivers, while offering the flexibility we need with a way to easily
> >>> detect the interface type at runtime and react on unknown/unsupported
> >>> types.
> >> 
> >> That's exactly what I was hoping could be avoided. If instead we modeled
> >> the interface type as a bus, we could for example have an lvds_bus along
> >> with an lvds_device and then use that as the natural place to store
> >> these properties. Much like we do for DSI.
> > 
> > And I believe that's what we should avoid ;-) First of all, let's not
> > forget that Linux models control busses, not data busses. DSI is a
> > special case as it combines the control and data busses, but in the
> > general case the same implementation isn't possible. An LVDS panel
> > controlled through I2C needs to be an I2C device sitting on an I2C bus.
> > 
> > Then, I believe it would make all drivers simpler if we had a single
> > object type to deal with, with proper abstractions for bus types. A
> > drm_panel that can model panels regardless of the data bus type, with one
> > operation that conveys bus-specific information, makes storing the objects
> > and communicating with them simpler than having to deal with different
> > kind of devices.
>
> Could you detail a bit what you mean by "single object type" ?
> 
> Is this about making a common abstraction class (by mean of
> drm_xxx and drm_xxx_funcs) that could represent any display device
> (drm_bridge, drm_panel, ...) ?

Exactly :-) This is similar to what exists in V4L, with a v4l2_subdev object 
able to model any kind of IP core or external chip.

I don't think we will get there in one go, but I'd like to start by merging 
drm_encoder and drm_bridge on the kernel side. Both objects model the same 
hardware, a drm_encoder on one board could be a drm_bridge on another one. 
From a userspace point of view drm_encoder won't go away, and we can't chain 
multiple encoders, so the change would be internal to the kernel only.

Then, as a next step, I believe using the same object to model panels would be 
a good idea, but there's no consensus on that.

On Wed, 16 Jul 2014 15:20:59 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Wednesday 16 July 2014 15:05:22 Boris BREZILLON wrote:
> > On Tue, 15 Jul 2014 13:07:58 +0200 Laurent Pinchart wrote:
> > > On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote:
> > >> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote:
> > >>> On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote:
> > >>>> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote:
> > >>>>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote:
> > >>>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > >>>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > >>>>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs
> > >>>>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides
> > >>>>>>>> a display controller device.
> > >>>>>>>> 
> > >>>>>>>> The HLCDC block provides a single RGB output port, and only
> > >>>>>>>> supports LCD panels connection to LCD panels for now.
> > >>>>>>>> 
> > >>>>>>>> The atmel,panel property link the HLCDC RGB output with the LCD
> > >>>>>>>> panel connected on this port (note that the HLCDC RGB connector
> > >>>>>>>> implementation makes use of the DRM panel framework).
> > >>>>>>>> 
> > >>>>>>>> Connection to other external devices (DRM bridges) might be added
> > >>>>>>>> later by mean of a new atmel,xxx (atmel,bridge) property.
> > >>>>>>>> 
> > >>>>>>>> Signed-off-by: Boris BREZILLON
> > >>>>>>>> <boris.brezillon@free-electrons.com>
> > >>>>>>>> ---
> > >>>>>>>> 
> > >>>>>>>>  .../devicetree/bindings/drm/atmel-hlcdc-dc.txt     | 59 ++++++++
> > >>>>>>>>  1 file changed, 59 insertions(+)
> > >>>>>>>>  create mode 100644
> > >>>>>>>>  Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > >>>>> 
> > >>>>> [snip]
> > >>>>> 
> > >>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > >>>>>>>> +   The first cell is a phandle to a DRM panel device
> > >>>>>>>> +   The second cell encodes the RGB mode, which can take the
> > >>>>>>>> following values:
> > >>>>>>>> +   * 0: RGB444
> > >>>>>>>> +   * 1: RGB565
> > >>>>>>>> +   * 2: RGB666
> > >>>>>>>> +   * 3: RGB888
> > >>>>>>> 
> > >>>>>>> These are properties of the panel and should be obtained from the
> > >>>>>>> panel directly rather than an additional cell in this specifier.
> > >>>>>> 
> > >>>>>> Okay.
> > >>>>>> What's the preferred way of doing this ?
> > >>>>>> What about defining an rgb-mode property in the panel node.
> > >>>>> 
> > >>>>> You could do that, but it won't help you much, as the HLCDC driver
> > >>>>> must not parse properties from the panel node. You should instead
> > >>>>> extend the drm_panel API with a function to retrieve panel
> > >>>>> properties. The HLCDC driver will then query the panel driver at
> > >>>>> runtime for the interface type. The panel driver will get the
> > >>>>> information from hardcoded data in the driver, from DT or possibly
> > >>>>> in some cases by querying the panel hardware directly.
> > >>>> 
> > >>>> My preference for this would be that we either add this to some
> > >>>> existing structure (struct drm_display_info seems like a good
> > >>>> candidate) or if the number of parameters grows out of hands, then
> > >>>> maybe even introduce a new type of device that's specific for the
> > >>>> interface. DRM panels are an abstraction for panels, that is,
> > >>>> interface-agnostic, and if we start exposing interface specific
> > >>>> parameters things will start to become very unwieldy.
> > >>> 
> > >>> I agree with the goal of keeping drm_panel interface-agnostic.
> > >>> However, one way or another, interface parameters will need to be
> > >>> communicated between the panel driver and the controller driver. My
> > >>> preference, if we need to extend the number and/or scope of parameters
> > >>> beyond what drm_display_info could reasonably contain, would be to
> > >>> implement a new drm_panel operation to query/configure interface
> > >>> parameters, using a structure that contains the interface type and a
> > >>> union of type-specific structures. This would keep the API generic in
> > >>> the sense of not requiring explicit knowledge of all interfaces in the
> > >>> drivers, while offering the flexibility we need with a way to easily
> > >>> detect the interface type at runtime and react on unknown/unsupported
> > >>> types.
> > >> 
> > >> That's exactly what I was hoping could be avoided. If instead we modeled
> > >> the interface type as a bus, we could for example have an lvds_bus along
> > >> with an lvds_device and then use that as the natural place to store
> > >> these properties. Much like we do for DSI.
> > > 
> > > And I believe that's what we should avoid ;-) First of all, let's not
> > > forget that Linux models control busses, not data busses. DSI is a
> > > special case as it combines the control and data busses, but in the
> > > general case the same implementation isn't possible. An LVDS panel
> > > controlled through I2C needs to be an I2C device sitting on an I2C bus.
> > > 
> > > Then, I believe it would make all drivers simpler if we had a single
> > > object type to deal with, with proper abstractions for bus types. A
> > > drm_panel that can model panels regardless of the data bus type, with one
> > > operation that conveys bus-specific information, makes storing the objects
> > > and communicating with them simpler than having to deal with different
> > > kind of devices.
> >
> > Could you detail a bit what you mean by "single object type" ?
> > 
> > Is this about making a common abstraction class (by mean of
> > drm_xxx and drm_xxx_funcs) that could represent any display device
> > (drm_bridge, drm_panel, ...) ?
> 
> Exactly :-) This is similar to what exists in V4L, with a v4l2_subdev object 
> able to model any kind of IP core or external chip.
> 
> I don't think we will get there in one go, but I'd like to start by merging 
> drm_encoder and drm_bridge on the kernel side. Both objects model the same 
> hardware, a drm_encoder on one board could be a drm_bridge on another one. 
> From a userspace point of view drm_encoder won't go away, and we can't chain 
> multiple encoders, so the change would be internal to the kernel only.
> 
> Then, as a next step, I believe using the same object to model panels would be 
> a good idea, but there's no consensus on that.
> 

I would be happy to help with that, but AFAICT, this is a huge work and
I'd like to get the HLCDC driver merged first ;-).

How about defining what DT bindings should look like (for the RGB/LVDS
output mode), and parsing this in atmel-hlcdc driver as a first step ?

Then we can define proper RGB/LVDS helper functions and the whole
drm_subdev abstraction you were talking about, and move the atmel-hlcdc
driver to this solution when it's ready.

Hi Thierry,

Oops, I missed this reply.

On Tue, 15 Jul 2014 12:31:37 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> [...]
> > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > [...]
> > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > > 
> > > I think it's better to refer to these using relative filenames. When the
> > > device tree bindings are moved out of the kernel tree, they may no
> > > longer use the same hierarchy.
> > 
> > Sure.
> > By relative path you mean ../../mfd/atmel-hlcdc.txt or just
> > mfd/atmel-hlcdc.txt ?
> 
> I think the former is more explicit.

Okay.

> 
> > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > +   The first cell is a phandle to a DRM panel device
> > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > +   * 0: RGB444
> > > > +   * 1: RGB565
> > > > +   * 2: RGB666
> > > > +   * 3: RGB888
> > > 
> > > These are properties of the panel and should be obtained from the panel
> > > directly rather than an additional cell in this specifier.
> > 
> > Okay.
> > What's the preferred way of doing this ?
> > What about defining an rgb-mode property in the panel node.
> 
> There's .bpc in struct drm_display_info, I suspect that it could be used
> for this. Alternatively, maybe we could extend the list of color formats
> that go into drm_display_info.color_formats? RGB444 is already covered.

I don't think this color_formats field is intended to represent data
stream format going through the bus.
Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma
subsampling rate) and not 12 bits signals (4 bits for each color).

Anyway I'll propose a patch series adding a new field to
drm_display_info to encode the mediabus format (as discussed with
Laurent and you).

> 
> Also, like Laurent said, this shouldn't go into the device tree, since
> it's already implied by the panel's compatible value, so we'd be
> duplicating information.

Again, this is not necessarily true (depending on your board design).
One can decide to connect an RGB888 panel on an RGB666 bus and connect
the missing pins to ground.

> 
> > BTW, have you received this series [1] adding support for the LCD panel
> > I'm testing this driver with.
> > 
> > [1] https://lkml.org/lkml/2014/6/5/612
> 
> I don't think I've seen it in my inbox, let me check my archives.
> 
> > > > +   The third cell encodes specific flags describing LCD signals configuration
> > > > +   (see Atmel's datasheet for a full description of these fields):
> > > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> > > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration
> > > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration
> > > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > > 
> > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the
> > > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
> > > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
> > > vsync_end of the same structure.
> > 
> > I agree with HSPOL and VSPOL.
> > 
> > > 
> > > As for the others, maybe if you could explain what exactly they are we
> > > may be able to find a better fit.
> > 
> > Atmel datasheets include several timing diagrams [2] (chapter "32.6.17
> > Output Timing Generation" page 603), and I think you will get more
> > informations from these diagrams than if I try to explain what I
> > understood ;-).
> 
> These look like knobs to tune the signal in a very fine-grained manner.
> To be honest, maybe the best way to solve this would be by omitting them
> for now and choose some default that's likely to work on most devices.
> Does the panel that you use specify how it expects HSYNC to be timed vs.
> VSYNC?


No it doesn't, and I agree that we should leave these specific timing
tweaks unimplemented until we really need them.

Regards,

Boris

On Fri, 18 Jul 2014 16:51:52 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Hi Thierry,
> 
> Oops, I missed this reply.
> 
> On Tue, 15 Jul 2014 12:31:37 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > [...]
> > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > [...]
> > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > > > 
> > > > I think it's better to refer to these using relative filenames. When the
> > > > device tree bindings are moved out of the kernel tree, they may no
> > > > longer use the same hierarchy.
> > > 
> > > Sure.
> > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just
> > > mfd/atmel-hlcdc.txt ?
> > 
> > I think the former is more explicit.
> 
> Okay.
> 
> > 
> > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > +   The first cell is a phandle to a DRM panel device
> > > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > > +   * 0: RGB444
> > > > > +   * 1: RGB565
> > > > > +   * 2: RGB666
> > > > > +   * 3: RGB888
> > > > 
> > > > These are properties of the panel and should be obtained from the panel
> > > > directly rather than an additional cell in this specifier.
> > > 
> > > Okay.
> > > What's the preferred way of doing this ?
> > > What about defining an rgb-mode property in the panel node.
> > 
> > There's .bpc in struct drm_display_info, I suspect that it could be used
> > for this. Alternatively, maybe we could extend the list of color formats
> > that go into drm_display_info.color_formats? RGB444 is already covered.
> 

I forgot to ask about bpc meaning. If, as I think, it means "bits per
color" then it cannot be used to encode RGB565 where green color is
encoded on 6 bits and red and blue are encoded on 5 bits. 

> I don't think this color_formats field is intended to represent data
> stream format going through the bus.
> Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma
> subsampling rate) and not 12 bits signals (4 bits for each color).
> 
> Anyway I'll propose a patch series adding a new field to
> drm_display_info to encode the mediabus format (as discussed with
> Laurent and you).
> 
> > 
> > Also, like Laurent said, this shouldn't go into the device tree, since
> > it's already implied by the panel's compatible value, so we'd be
> > duplicating information.
> 
> Again, this is not necessarily true (depending on your board design).
> One can decide to connect an RGB888 panel on an RGB666 bus and connect
> the missing pins to ground.
> 
> > 
> > > BTW, have you received this series [1] adding support for the LCD panel
> > > I'm testing this driver with.
> > > 
> > > [1] https://lkml.org/lkml/2014/6/5/612
> > 
> > I don't think I've seen it in my inbox, let me check my archives.
> > 
> > > > > +   The third cell encodes specific flags describing LCD signals configuration
> > > > > +   (see Atmel's datasheet for a full description of these fields):
> > > > > +   * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity
> > > > > +   * bit 1: VSPOL: Vertical Synchronization Pulse Polarity
> > > > > +   * bit 2: VSPDLYS: Vertical Synchronization Pulse Start
> > > > > +   * bit 3: VSPDLYE: Vertical Synchronization Pulse End
> > > > > +   * bit 4: DISPPOL: Display Signal Polarity
> > > > > +   * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization
> > > > > +   * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration
> > > > > +   * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration
> > > > > +   * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time
> > > > 
> > > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the
> > > > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And
> > > > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and
> > > > vsync_end of the same structure.
> > > 
> > > I agree with HSPOL and VSPOL.
> > > 
> > > > 
> > > > As for the others, maybe if you could explain what exactly they are we
> > > > may be able to find a better fit.
> > > 
> > > Atmel datasheets include several timing diagrams [2] (chapter "32.6.17
> > > Output Timing Generation" page 603), and I think you will get more
> > > informations from these diagrams than if I try to explain what I
> > > understood ;-).
> > 
> > These look like knobs to tune the signal in a very fine-grained manner.
> > To be honest, maybe the best way to solve this would be by omitting them
> > for now and choose some default that's likely to work on most devices.
> > Does the panel that you use specify how it expects HSYNC to be timed vs.
> > VSYNC?
> 
> 
> No it doesn't, and I agree that we should leave these specific timing
> tweaks unimplemented until we really need them.
> 
> Regards,
> 
> Boris
> 
>

On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
[...]
> > > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > > +   The first cell is a phandle to a DRM panel device
> > > > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > > > +   * 0: RGB444
> > > > > > +   * 1: RGB565
> > > > > > +   * 2: RGB666
> > > > > > +   * 3: RGB888
> > > > > 
> > > > > These are properties of the panel and should be obtained from the panel
> > > > > directly rather than an additional cell in this specifier.
> > > > 
> > > > Okay.
> > > > What's the preferred way of doing this ?
> > > > What about defining an rgb-mode property in the panel node.
> > > 
> > > There's .bpc in struct drm_display_info, I suspect that it could be used
> > > for this. Alternatively, maybe we could extend the list of color formats
> > > that go into drm_display_info.color_formats? RGB444 is already covered.
> > 
> 
> I forgot to ask about bpc meaning. If, as I think, it means "bits per
> color" then it cannot be used to encode RGB565 where green color is
> encoded on 6 bits and red and blue are encoded on 5 bits. 

Yes, I agree that bps is not a good fit for what you need here.

Thierry

On Mon, 21 Jul 2014 10:59:12 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> [...]
> > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > > > +   The first cell is a phandle to a DRM panel device
> > > > > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > > > > +   * 0: RGB444
> > > > > > > +   * 1: RGB565
> > > > > > > +   * 2: RGB666
> > > > > > > +   * 3: RGB888
> > > > > > 
> > > > > > These are properties of the panel and should be obtained from the panel
> > > > > > directly rather than an additional cell in this specifier.
> > > > > 
> > > > > Okay.
> > > > > What's the preferred way of doing this ?
> > > > > What about defining an rgb-mode property in the panel node.
> > > > 
> > > > There's .bpc in struct drm_display_info, I suspect that it could be used
> > > > for this. Alternatively, maybe we could extend the list of color formats
> > > > that go into drm_display_info.color_formats? RGB444 is already covered.
> > > 
> > 
> > I forgot to ask about bpc meaning. If, as I think, it means "bits per
> > color" then it cannot be used to encode RGB565 where green color is
> > encoded on 6 bits and red and blue are encoded on 5 bits. 
> 
> Yes, I agree that bps is not a good fit for what you need here.

Okay, then I think we can replace bpc and color_formats by a bus_formats
table containing all supported formats, and use an enum (something
similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list
the available formats.

As this implies quite a few changes in crtc core and some drm drivers
(nouveau, i915 and radeon), I'd like to be sure this is what both of you
had in mind.

Best Regards,

Boris

[1]http://lxr.free-electrons.com/source/include/uapi/linux/v4l2-mediabus.h#L37

Hi Boris,

On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote:
> > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> > [...]
> > 
> >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> >>>>>>> +   The first cell is a phandle to a DRM panel device
> >>>>>>> +   The second cell encodes the RGB mode, which can take the
> >>>>>>> following values:
> >>>>>>> +   * 0: RGB444
> >>>>>>> +   * 1: RGB565
> >>>>>>> +   * 2: RGB666
> >>>>>>> +   * 3: RGB888
> >>>>>> 
> >>>>>> These are properties of the panel and should be obtained from
> >>>>>> the panel directly rather than an additional cell in this specifier.
> >>>>> 
> >>>>> Okay.
> >>>>> What's the preferred way of doing this ?
> >>>>> What about defining an rgb-mode property in the panel node.
> >>>> 
> >>>> There's .bpc in struct drm_display_info, I suspect that it could be
> >>>> used for this. Alternatively, maybe we could extend the list of color
> >>>> formats that go into drm_display_info.color_formats? RGB444 is already
> >>>> covered.
> >> 
> >> I forgot to ask about bpc meaning. If, as I think, it means "bits per
> >> color" then it cannot be used to encode RGB565 where green color is
> >> encoded on 6 bits and red and blue are encoded on 5 bits.
> > 
> > Yes, I agree that bps is not a good fit for what you need here.
> 
> Okay, then I think we can replace bpc and color_formats by a bus_formats
> table containing all supported formats, and use an enum (something
> similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list
> the available formats.
> 
> As this implies quite a few changes in crtc core and some drm drivers
> (nouveau, i915 and radeon), I'd like to be sure this is what both of you
> had in mind.

I think it is, but just to make sure I understand you correctly, could you 
just show how the drm_display_info structure would look like ?

On Mon, 21 Jul 2014 11:32:55 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote:
> > > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> > > [...]
> > > 
> > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > >>>>>>> +   The first cell is a phandle to a DRM panel device
> > >>>>>>> +   The second cell encodes the RGB mode, which can take the
> > >>>>>>> following values:
> > >>>>>>> +   * 0: RGB444
> > >>>>>>> +   * 1: RGB565
> > >>>>>>> +   * 2: RGB666
> > >>>>>>> +   * 3: RGB888
> > >>>>>> 
> > >>>>>> These are properties of the panel and should be obtained from
> > >>>>>> the panel directly rather than an additional cell in this specifier.
> > >>>>> 
> > >>>>> Okay.
> > >>>>> What's the preferred way of doing this ?
> > >>>>> What about defining an rgb-mode property in the panel node.
> > >>>> 
> > >>>> There's .bpc in struct drm_display_info, I suspect that it could be
> > >>>> used for this. Alternatively, maybe we could extend the list of color
> > >>>> formats that go into drm_display_info.color_formats? RGB444 is already
> > >>>> covered.
> > >> 
> > >> I forgot to ask about bpc meaning. If, as I think, it means "bits per
> > >> color" then it cannot be used to encode RGB565 where green color is
> > >> encoded on 6 bits and red and blue are encoded on 5 bits.
> > > 
> > > Yes, I agree that bps is not a good fit for what you need here.
> > 
> > Okay, then I think we can replace bpc and color_formats by a bus_formats
> > table containing all supported formats, and use an enum (something
> > similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list
> > the available formats.
> > 
> > As this implies quite a few changes in crtc core and some drm drivers
> > (nouveau, i915 and radeon), I'd like to be sure this is what both of you
> > had in mind.
> 
> I think it is, but just to make sure I understand you correctly, could you 
> just show how the drm_display_info structure would look like ?
> 

The new drm_display_info structure should look like this [2] (except
that color_formats and bpc have not be removed yet), and [1] is just
here to show how the video_bus_format enum would look like.

[1] http://code.bulix.org/rfd0yx-86557
[2] http://code.bulix.org/7n03b4-86556

On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 11:32:55 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> 
> > Hi Boris,
> > 
> > On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote:
> > > On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote:
> > > > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> > > > [...]
> > > > 
> > > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > > >>>>>>> +   The first cell is a phandle to a DRM panel device
> > > >>>>>>> +   The second cell encodes the RGB mode, which can take the
> > > >>>>>>> following values:
> > > >>>>>>> +   * 0: RGB444
> > > >>>>>>> +   * 1: RGB565
> > > >>>>>>> +   * 2: RGB666
> > > >>>>>>> +   * 3: RGB888
> > > >>>>>> 
> > > >>>>>> These are properties of the panel and should be obtained from
> > > >>>>>> the panel directly rather than an additional cell in this specifier.
> > > >>>>> 
> > > >>>>> Okay.
> > > >>>>> What's the preferred way of doing this ?
> > > >>>>> What about defining an rgb-mode property in the panel node.
> > > >>>> 
> > > >>>> There's .bpc in struct drm_display_info, I suspect that it could be
> > > >>>> used for this. Alternatively, maybe we could extend the list of color
> > > >>>> formats that go into drm_display_info.color_formats? RGB444 is already
> > > >>>> covered.
> > > >> 
> > > >> I forgot to ask about bpc meaning. If, as I think, it means "bits per
> > > >> color" then it cannot be used to encode RGB565 where green color is
> > > >> encoded on 6 bits and red and blue are encoded on 5 bits.
> > > > 
> > > > Yes, I agree that bps is not a good fit for what you need here.
> > > 
> > > Okay, then I think we can replace bpc and color_formats by a bus_formats
> > > table containing all supported formats, and use an enum (something
> > > similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list
> > > the available formats.
> > > 
> > > As this implies quite a few changes in crtc core and some drm drivers
> > > (nouveau, i915 and radeon), I'd like to be sure this is what both of you
> > > had in mind.
> > 
> > I think it is, but just to make sure I understand you correctly, could you 
> > just show how the drm_display_info structure would look like ?
> > 
> 
> The new drm_display_info structure should look like this [2] (except
> that color_formats and bpc have not be removed yet), and [1] is just
> here to show how the video_bus_format enum would look like.
> 
> [1] http://code.bulix.org/rfd0yx-86557
> [2] http://code.bulix.org/7n03b4-86556

Quoting from your paste:

	+   const enum video_bus_format *bus_formats;
	+   int nbus_formats;

Do we really need more than one?

Thierry

On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote:
> Hi Thierry,
> 
> Oops, I missed this reply.
> 
> On Tue, 15 Jul 2014 12:31:37 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > [...]
> > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > [...]
> > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > > > 
> > > > I think it's better to refer to these using relative filenames. When the
> > > > device tree bindings are moved out of the kernel tree, they may no
> > > > longer use the same hierarchy.
> > > 
> > > Sure.
> > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just
> > > mfd/atmel-hlcdc.txt ?
> > 
> > I think the former is more explicit.
> 
> Okay.
> 
> > 
> > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > +   The first cell is a phandle to a DRM panel device
> > > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > > +   * 0: RGB444
> > > > > +   * 1: RGB565
> > > > > +   * 2: RGB666
> > > > > +   * 3: RGB888
> > > > 
> > > > These are properties of the panel and should be obtained from the panel
> > > > directly rather than an additional cell in this specifier.
> > > 
> > > Okay.
> > > What's the preferred way of doing this ?
> > > What about defining an rgb-mode property in the panel node.
> > 
> > There's .bpc in struct drm_display_info, I suspect that it could be used
> > for this. Alternatively, maybe we could extend the list of color formats
> > that go into drm_display_info.color_formats? RGB444 is already covered.
> 
> I don't think this color_formats field is intended to represent data
> stream format going through the bus.
> Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma
> subsampling rate) and not 12 bits signals (4 bits for each color).
> 
> Anyway I'll propose a patch series adding a new field to
> drm_display_info to encode the mediabus format (as discussed with
> Laurent and you).
> 
> > 
> > Also, like Laurent said, this shouldn't go into the device tree, since
> > it's already implied by the panel's compatible value, so we'd be
> > duplicating information.
> 
> Again, this is not necessarily true (depending on your board design).
> One can decide to connect an RGB888 panel on an RGB666 bus and connect
> the missing pins to ground.

I think in that case the board design itself could be considered as an
RGB888 to RGB666 bridge, and I think that's what the device tree should
be describing rather than a panel with a variable number of input
formats.

Thierry

Hi Thierry,

On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart wrote:
> >> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote:
> >>> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote:
> >>>> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> >>>> [...]
> >>>> 
> >>>>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> >>>>>>>>>> +   The first cell is a phandle to a DRM panel device
> >>>>>>>>>> +   The second cell encodes the RGB mode, which can take the
> >>>>>>>>>> following values:
> >>>>>>>>>> +   * 0: RGB444
> >>>>>>>>>> +   * 1: RGB565
> >>>>>>>>>> +   * 2: RGB666
> >>>>>>>>>> +   * 3: RGB888
> >>>>>>>>> 
> >>>>>>>>> These are properties of the panel and should be obtained from
> >>>>>>>>> the panel directly rather than an additional cell in this
> >>>>>>>>> specifier.
> >>>>>>>> 
> >>>>>>>> Okay.
> >>>>>>>> What's the preferred way of doing this ?
> >>>>>>>> What about defining an rgb-mode property in the panel node.
> >>>>>>> 
> >>>>>>> There's .bpc in struct drm_display_info, I suspect that it could
> >>>>>>> be used for this. Alternatively, maybe we could extend the list
> >>>>>>> of color formats that go into drm_display_info.color_formats?
> >>>>>>> RGB444 is already covered.
> >>>>> 
> >>>>> I forgot to ask about bpc meaning. If, as I think, it means "bits
> >>>>> per color" then it cannot be used to encode RGB565 where green
> >>>>> color is encoded on 6 bits and red and blue are encoded on 5 bits.
> >>>> 
> >>>> Yes, I agree that bps is not a good fit for what you need here.
> >>> 
> >>> Okay, then I think we can replace bpc and color_formats by a
> >>> bus_formats table containing all supported formats, and use an enum
> >>> (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h
> >>> [1]) to list the available formats.
> >>> 
> >>> As this implies quite a few changes in crtc core and some drm drivers
> >>> (nouveau, i915 and radeon), I'd like to be sure this is what both of
> >>> you had in mind.
> >> 
> >> I think it is, but just to make sure I understand you correctly, could
> >> you just show how the drm_display_info structure would look like ?
> > 
> > The new drm_display_info structure should look like this [2] (except
> > that color_formats and bpc have not be removed yet), and [1] is just
> > here to show how the video_bus_format enum would look like.
> > 
> > [1] http://code.bulix.org/rfd0yx-86557
> > [2] http://code.bulix.org/7n03b4-86556
> 
> Quoting from your paste:
> 
> 	+   const enum video_bus_format *bus_formats;
> 	+   int nbus_formats;
> 
> Do we really need more than one?

We do if we want to replace the color_formats and bpc fields.

On Mon, 21 Jul 2014 14:15:16 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote:
> > Hi Thierry,
> > 
> > Oops, I missed this reply.
> > 
> > On Tue, 15 Jul 2014 12:31:37 +0200
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > > [...]
> > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > > [...]
> > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > > > > 
> > > > > I think it's better to refer to these using relative filenames. When the
> > > > > device tree bindings are moved out of the kernel tree, they may no
> > > > > longer use the same hierarchy.
> > > > 
> > > > Sure.
> > > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just
> > > > mfd/atmel-hlcdc.txt ?
> > > 
> > > I think the former is more explicit.
> > 
> > Okay.
> > 
> > > 
> > > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > > +   The first cell is a phandle to a DRM panel device
> > > > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > > > +   * 0: RGB444
> > > > > > +   * 1: RGB565
> > > > > > +   * 2: RGB666
> > > > > > +   * 3: RGB888
> > > > > 
> > > > > These are properties of the panel and should be obtained from the panel
> > > > > directly rather than an additional cell in this specifier.
> > > > 
> > > > Okay.
> > > > What's the preferred way of doing this ?
> > > > What about defining an rgb-mode property in the panel node.
> > > 
> > > There's .bpc in struct drm_display_info, I suspect that it could be used
> > > for this. Alternatively, maybe we could extend the list of color formats
> > > that go into drm_display_info.color_formats? RGB444 is already covered.
> > 
> > I don't think this color_formats field is intended to represent data
> > stream format going through the bus.
> > Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma
> > subsampling rate) and not 12 bits signals (4 bits for each color).
> > 
> > Anyway I'll propose a patch series adding a new field to
> > drm_display_info to encode the mediabus format (as discussed with
> > Laurent and you).
> > 
> > > 
> > > Also, like Laurent said, this shouldn't go into the device tree, since
> > > it's already implied by the panel's compatible value, so we'd be
> > > duplicating information.
> > 
> > Again, this is not necessarily true (depending on your board design).
> > One can decide to connect an RGB888 panel on an RGB666 bus and connect
> > the missing pins to ground.
> 
> I think in that case the board design itself could be considered as an
> RGB888 to RGB666 bridge, and I think that's what the device tree should
> be describing rather than a panel with a variable number of input
> formats.

So, you're suggesting to add an RGB to RGB drm_bridge driver (and
the appropriate DT bindings) to handle this case, right ?

I don't know much about drm bridges, but I'll take a look.

Anyway, at the moment I don't have such hardware (one connecting an
RGB888 panel on an RGB666 bus), I was just wondering how I could
represent it ;-).

Thanks,

Boris

On Mon, 21 Jul 2014 14:16:42 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Thierry,
> 
> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > > On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart wrote:
> > >> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote:
> > >>> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote:
> > >>>> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> > >>>> [...]
> > >>>> 
> > >>>>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > >>>>>>>>>> +   The first cell is a phandle to a DRM panel device
> > >>>>>>>>>> +   The second cell encodes the RGB mode, which can take the
> > >>>>>>>>>> following values:
> > >>>>>>>>>> +   * 0: RGB444
> > >>>>>>>>>> +   * 1: RGB565
> > >>>>>>>>>> +   * 2: RGB666
> > >>>>>>>>>> +   * 3: RGB888
> > >>>>>>>>> 
> > >>>>>>>>> These are properties of the panel and should be obtained from
> > >>>>>>>>> the panel directly rather than an additional cell in this
> > >>>>>>>>> specifier.
> > >>>>>>>> 
> > >>>>>>>> Okay.
> > >>>>>>>> What's the preferred way of doing this ?
> > >>>>>>>> What about defining an rgb-mode property in the panel node.
> > >>>>>>> 
> > >>>>>>> There's .bpc in struct drm_display_info, I suspect that it could
> > >>>>>>> be used for this. Alternatively, maybe we could extend the list
> > >>>>>>> of color formats that go into drm_display_info.color_formats?
> > >>>>>>> RGB444 is already covered.
> > >>>>> 
> > >>>>> I forgot to ask about bpc meaning. If, as I think, it means "bits
> > >>>>> per color" then it cannot be used to encode RGB565 where green
> > >>>>> color is encoded on 6 bits and red and blue are encoded on 5 bits.
> > >>>> 
> > >>>> Yes, I agree that bps is not a good fit for what you need here.
> > >>> 
> > >>> Okay, then I think we can replace bpc and color_formats by a
> > >>> bus_formats table containing all supported formats, and use an enum
> > >>> (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h
> > >>> [1]) to list the available formats.
> > >>> 
> > >>> As this implies quite a few changes in crtc core and some drm drivers
> > >>> (nouveau, i915 and radeon), I'd like to be sure this is what both of
> > >>> you had in mind.
> > >> 
> > >> I think it is, but just to make sure I understand you correctly, could
> > >> you just show how the drm_display_info structure would look like ?
> > > 
> > > The new drm_display_info structure should look like this [2] (except
> > > that color_formats and bpc have not be removed yet), and [1] is just
> > > here to show how the video_bus_format enum would look like.
> > > 
> > > [1] http://code.bulix.org/rfd0yx-86557
> > > [2] http://code.bulix.org/7n03b4-86556
> > 
> > Quoting from your paste:
> > 
> > 	+   const enum video_bus_format *bus_formats;
> > 	+   int nbus_formats;
> > 
> > Do we really need more than one?
> 
> We do if we want to replace the color_formats and bpc fields.
> 

Yes, that's what I was about to answer :-).

On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 14:16:42 +0200
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> 
> > Hi Thierry,
> > 
> > On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > > On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > > > On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart wrote:
> > > >> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote:
> > > >>> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote:
> > > >>>> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote:
> > > >>>> [...]
> > > >>>> 
> > > >>>>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > > >>>>>>>>>> +   The first cell is a phandle to a DRM panel device
> > > >>>>>>>>>> +   The second cell encodes the RGB mode, which can take the
> > > >>>>>>>>>> following values:
> > > >>>>>>>>>> +   * 0: RGB444
> > > >>>>>>>>>> +   * 1: RGB565
> > > >>>>>>>>>> +   * 2: RGB666
> > > >>>>>>>>>> +   * 3: RGB888
> > > >>>>>>>>> 
> > > >>>>>>>>> These are properties of the panel and should be obtained from
> > > >>>>>>>>> the panel directly rather than an additional cell in this
> > > >>>>>>>>> specifier.
> > > >>>>>>>> 
> > > >>>>>>>> Okay.
> > > >>>>>>>> What's the preferred way of doing this ?
> > > >>>>>>>> What about defining an rgb-mode property in the panel node.
> > > >>>>>>> 
> > > >>>>>>> There's .bpc in struct drm_display_info, I suspect that it could
> > > >>>>>>> be used for this. Alternatively, maybe we could extend the list
> > > >>>>>>> of color formats that go into drm_display_info.color_formats?
> > > >>>>>>> RGB444 is already covered.
> > > >>>>> 
> > > >>>>> I forgot to ask about bpc meaning. If, as I think, it means "bits
> > > >>>>> per color" then it cannot be used to encode RGB565 where green
> > > >>>>> color is encoded on 6 bits and red and blue are encoded on 5 bits.
> > > >>>> 
> > > >>>> Yes, I agree that bps is not a good fit for what you need here.
> > > >>> 
> > > >>> Okay, then I think we can replace bpc and color_formats by a
> > > >>> bus_formats table containing all supported formats, and use an enum
> > > >>> (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h
> > > >>> [1]) to list the available formats.
> > > >>> 
> > > >>> As this implies quite a few changes in crtc core and some drm drivers
> > > >>> (nouveau, i915 and radeon), I'd like to be sure this is what both of
> > > >>> you had in mind.
> > > >> 
> > > >> I think it is, but just to make sure I understand you correctly, could
> > > >> you just show how the drm_display_info structure would look like ?
> > > > 
> > > > The new drm_display_info structure should look like this [2] (except
> > > > that color_formats and bpc have not be removed yet), and [1] is just
> > > > here to show how the video_bus_format enum would look like.
> > > > 
> > > > [1] http://code.bulix.org/rfd0yx-86557
> > > > [2] http://code.bulix.org/7n03b4-86556
> > > 
> > > Quoting from your paste:
> > > 
> > > 	+   const enum video_bus_format *bus_formats;
> > > 	+   int nbus_formats;
> > > 
> > > Do we really need more than one?
> > 
> > We do if we want to replace the color_formats and bpc fields.
> > 
> 
> Yes, that's what I was about to answer :-).

Maybe we don't need to replace color_formats and bpc field immediately.
That could be done in a follow-up patch.

Thierry

On Mon, Jul 21, 2014 at 02:33:21PM +0200, Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 14:15:16 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote:
> > > Hi Thierry,
> > > 
> > > Oops, I missed this reply.
> > > 
> > > On Tue, 15 Jul 2014 12:31:37 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > > 
> > > > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > > > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> > > > [...]
> > > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt
> > > > > > [...]
> > > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device.
> > > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details.
> > > > > > 
> > > > > > I think it's better to refer to these using relative filenames. When the
> > > > > > device tree bindings are moved out of the kernel tree, they may no
> > > > > > longer use the same hierarchy.
> > > > > 
> > > > > Sure.
> > > > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just
> > > > > mfd/atmel-hlcdc.txt ?
> > > > 
> > > > I think the former is more explicit.
> > > 
> > > Okay.
> > > 
> > > > 
> > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters.
> > > > > > > +   The first cell is a phandle to a DRM panel device
> > > > > > > +   The second cell encodes the RGB mode, which can take the following values:
> > > > > > > +   * 0: RGB444
> > > > > > > +   * 1: RGB565
> > > > > > > +   * 2: RGB666
> > > > > > > +   * 3: RGB888
> > > > > > 
> > > > > > These are properties of the panel and should be obtained from the panel
> > > > > > directly rather than an additional cell in this specifier.
> > > > > 
> > > > > Okay.
> > > > > What's the preferred way of doing this ?
> > > > > What about defining an rgb-mode property in the panel node.
> > > > 
> > > > There's .bpc in struct drm_display_info, I suspect that it could be used
> > > > for this. Alternatively, maybe we could extend the list of color formats
> > > > that go into drm_display_info.color_formats? RGB444 is already covered.
> > > 
> > > I don't think this color_formats field is intended to represent data
> > > stream format going through the bus.
> > > Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma
> > > subsampling rate) and not 12 bits signals (4 bits for each color).
> > > 
> > > Anyway I'll propose a patch series adding a new field to
> > > drm_display_info to encode the mediabus format (as discussed with
> > > Laurent and you).
> > > 
> > > > 
> > > > Also, like Laurent said, this shouldn't go into the device tree, since
> > > > it's already implied by the panel's compatible value, so we'd be
> > > > duplicating information.
> > > 
> > > Again, this is not necessarily true (depending on your board design).
> > > One can decide to connect an RGB888 panel on an RGB666 bus and connect
> > > the missing pins to ground.
> > 
> > I think in that case the board design itself could be considered as an
> > RGB888 to RGB666 bridge, and I think that's what the device tree should
> > be describing rather than a panel with a variable number of input
> > formats.
> 
> So, you're suggesting to add an RGB to RGB drm_bridge driver (and
> the appropriate DT bindings) to handle this case, right ?

Yes, exactly.

Thierry

Hi Thierry,

On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> >> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> >>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:

[snip]

> >>>> The new drm_display_info structure should look like this [2] (except
> >>>> that color_formats and bpc have not be removed yet), and [1] is just
> >>>> here to show how the video_bus_format enum would look like.
> >>>> 
> >>>> [1] http://code.bulix.org/rfd0yx-86557
> >>>> [2] http://code.bulix.org/7n03b4-86556
> >>> 
> >>> Quoting from your paste:
> >>> 	+   const enum video_bus_format *bus_formats;
> >>> 	+   int nbus_formats;
> >>> 
> >>> Do we really need more than one?
> >> 
> >> We do if we want to replace the color_formats and bpc fields.
> > 
> > Yes, that's what I was about to answer :-).
> 
> Maybe we don't need to replace color_formats and bpc field immediately.
> That could be done in a follow-up patch.

We don't need to replace them right now, but we should at least agree on how 
to replace them. Introducing a new field that would need to be replaced in the 
near future when removing color_formats and bpc would be a waste of time.

Hi Thierry,

On Monday 21 July 2014 14:56:26 Thierry Reding wrote:
> On Mon, Jul 21, 2014 at 02:33:21PM +0200, Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 14:15:16 +0200 Thierry Reding wrote:
> >> On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote:
> >>> On Tue, 15 Jul 2014 12:31:37 +0200 Thierry Reding wrote:
> >>>> On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> >>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> >>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:

[snip]

> >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> >>>>>>> +   The first cell is a phandle to a DRM panel device
> >>>>>>> +   The second cell encodes the RGB mode, which can take the
> >>>>>>> following values: +   * 0: RGB444
> >>>>>>> +   * 1: RGB565
> >>>>>>> +   * 2: RGB666
> >>>>>>> +   * 3: RGB888
> >>>>>> 
> >>>>>> These are properties of the panel and should be obtained from
> >>>>>> the panel directly rather than an additional cell in this
> >>>>>> specifier.
> >>>>> 
> >>>>> Okay.
> >>>>> What's the preferred way of doing this ?
> >>>>> What about defining an rgb-mode property in the panel node.

[snip]

> >>>> Also, like Laurent said, this shouldn't go into the device tree,
> >>>> since it's already implied by the panel's compatible value, so we'd
> >>>> be duplicating information.
> >>> 
> >>> Again, this is not necessarily true (depending on your board design).
> >>> One can decide to connect an RGB888 panel on an RGB666 bus and connect
> >>> the missing pins to ground.
> >> 
> >> I think in that case the board design itself could be considered as an
> >> RGB888 to RGB666 bridge, and I think that's what the device tree should
> >> be describing rather than a panel with a variable number of input
> >> formats.
> > 
> > So, you're suggesting to add an RGB to RGB drm_bridge driver (and
> > the appropriate DT bindings) to handle this case, right ?
> 
> Yes, exactly.

Wouldn't it be possible to implement RGB666 -> RGB888 support in a less 
complex way ? A standalone driver to describe signal routing seems like an 
overly complex solution to me. I would prefer making the routing a properly of 
the link instead of a separate device.

On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> > On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > > On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> > >> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > >>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> 
> [snip]
> 
> > >>>> The new drm_display_info structure should look like this [2] (except
> > >>>> that color_formats and bpc have not be removed yet), and [1] is just
> > >>>> here to show how the video_bus_format enum would look like.
> > >>>> 
> > >>>> [1] http://code.bulix.org/rfd0yx-86557
> > >>>> [2] http://code.bulix.org/7n03b4-86556
> > >>> 
> > >>> Quoting from your paste:
> > >>> 	+   const enum video_bus_format *bus_formats;
> > >>> 	+   int nbus_formats;
> > >>> 
> > >>> Do we really need more than one?
> > >> 
> > >> We do if we want to replace the color_formats and bpc fields.
> > > 
> > > Yes, that's what I was about to answer :-).
> > 
> > Maybe we don't need to replace color_formats and bpc field immediately.
> > That could be done in a follow-up patch.
> 
> We don't need to replace them right now, but we should at least agree on how 
> to replace them. Introducing a new field that would need to be replaced in the 
> near future when removing color_formats and bpc would be a waste of time.

Sure. One of the problems I see with replacing color_formats and bpc
with the above is that some of the bits within color_formats are set
when the EDID is parsed. That implies that if they are replaced with
an array of formats, the array would need to be reallocated during EDID
parsing. That sounds like ugliness.

But if you can find a nice way to make it work that'd be great.

Thierry

On Mon, Jul 21, 2014 at 03:26:12PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Monday 21 July 2014 14:56:26 Thierry Reding wrote:
> > On Mon, Jul 21, 2014 at 02:33:21PM +0200, Boris BREZILLON wrote:
> > > On Mon, 21 Jul 2014 14:15:16 +0200 Thierry Reding wrote:
> > >> On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote:
> > >>> On Tue, 15 Jul 2014 12:31:37 +0200 Thierry Reding wrote:
> > >>>> On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote:
> > >>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote:
> > >>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote:
> 
> [snip]
> 
> > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters.
> > >>>>>>> +   The first cell is a phandle to a DRM panel device
> > >>>>>>> +   The second cell encodes the RGB mode, which can take the
> > >>>>>>> following values: +   * 0: RGB444
> > >>>>>>> +   * 1: RGB565
> > >>>>>>> +   * 2: RGB666
> > >>>>>>> +   * 3: RGB888
> > >>>>>> 
> > >>>>>> These are properties of the panel and should be obtained from
> > >>>>>> the panel directly rather than an additional cell in this
> > >>>>>> specifier.
> > >>>>> 
> > >>>>> Okay.
> > >>>>> What's the preferred way of doing this ?
> > >>>>> What about defining an rgb-mode property in the panel node.
> 
> [snip]
> 
> > >>>> Also, like Laurent said, this shouldn't go into the device tree,
> > >>>> since it's already implied by the panel's compatible value, so we'd
> > >>>> be duplicating information.
> > >>> 
> > >>> Again, this is not necessarily true (depending on your board design).
> > >>> One can decide to connect an RGB888 panel on an RGB666 bus and connect
> > >>> the missing pins to ground.
> > >> 
> > >> I think in that case the board design itself could be considered as an
> > >> RGB888 to RGB666 bridge, and I think that's what the device tree should
> > >> be describing rather than a panel with a variable number of input
> > >> formats.
> > > 
> > > So, you're suggesting to add an RGB to RGB drm_bridge driver (and
> > > the appropriate DT bindings) to handle this case, right ?
> > 
> > Yes, exactly.
> 
> Wouldn't it be possible to implement RGB666 -> RGB888 support in a less 
> complex way ? A standalone driver to describe signal routing seems like an 
> overly complex solution to me. I would prefer making the routing a properly of 
> the link instead of a separate device.

I don't think the above is overly complex. After all the panel expects
RGB888, so it makes no sense to make it "configurable" to anything else.
Similarly if the encoder or bridge provides RGB666 then that's a fixed
function, too. So to represent this combination accurately you'll need
some form of translation entity inbetween, and it may just as well be a
bridge than something custom for that particular link.

Thierry

On Mon, 21 Jul 2014 15:30:35 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> > > On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > > > On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> > > >> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > > >>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > 
> > [snip]
> > 
> > > >>>> The new drm_display_info structure should look like this [2] (except
> > > >>>> that color_formats and bpc have not be removed yet), and [1] is just
> > > >>>> here to show how the video_bus_format enum would look like.
> > > >>>> 
> > > >>>> [1] http://code.bulix.org/rfd0yx-86557
> > > >>>> [2] http://code.bulix.org/7n03b4-86556
> > > >>> 
> > > >>> Quoting from your paste:
> > > >>> 	+   const enum video_bus_format *bus_formats;
> > > >>> 	+   int nbus_formats;
> > > >>> 
> > > >>> Do we really need more than one?
> > > >> 
> > > >> We do if we want to replace the color_formats and bpc fields.
> > > > 
> > > > Yes, that's what I was about to answer :-).
> > > 
> > > Maybe we don't need to replace color_formats and bpc field immediately.
> > > That could be done in a follow-up patch.
> > 
> > We don't need to replace them right now, but we should at least agree on how 
> > to replace them. Introducing a new field that would need to be replaced in the 
> > near future when removing color_formats and bpc would be a waste of time.
> 
> Sure. One of the problems I see with replacing color_formats and bpc
> with the above is that some of the bits within color_formats are set
> when the EDID is parsed. That implies that if they are replaced with
> an array of formats, the array would need to be reallocated during EDID
> parsing. That sounds like ugliness.
> 
> But if you can find a nice way to make it work that'd be great.

How about using a list instead of an array ?
This way we can add elements to this list when parsing the EDID.

Or we can just define a maximum size for the bus_formats array when
retrieving this info from EDID. If I'm correct we have at most 18 bus
formats:
 - 3 color formats:
   * RGB 4:4:4
   * YCbCr 4:4:4
   * YCbCr 4:4:2
 - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color

Hi Boris,

On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> >>
> >> [snip]
> >> 
> >>>>>>> The new drm_display_info structure should look like this [2]
> >>>>>>> (except that color_formats and bpc have not be removed yet), and
> >>>>>>> [1] is just here to show how the video_bus_format enum would look
> >>>>>>> like.
> >>>>>>> 
> >>>>>>> [1] http://code.bulix.org/rfd0yx-86557
> >>>>>>> [2] http://code.bulix.org/7n03b4-86556
> >>>>>> 
> >>>>>> Quoting from your paste:
> >>>>>> 	+   const enum video_bus_format *bus_formats;
> >>>>>> 	+   int nbus_formats;
> >>>>>> 
> >>>>>> Do we really need more than one?
> >>>>> 
> >>>>> We do if we want to replace the color_formats and bpc fields.
> >>>> 
> >>>> Yes, that's what I was about to answer :-).
> >>> 
> >>> Maybe we don't need to replace color_formats and bpc field
> >>> immediately. That could be done in a follow-up patch.
> >> 
> >> We don't need to replace them right now, but we should at least agree on
> >> how to replace them. Introducing a new field that would need to be
> >> replaced in the near future when removing color_formats and bpc would
> >> be a waste of time.
> >
> > Sure. One of the problems I see with replacing color_formats and bpc
> > with the above is that some of the bits within color_formats are set
> > when the EDID is parsed. That implies that if they are replaced with
> > an array of formats, the array would need to be reallocated during EDID
> > parsing. That sounds like ugliness.
> > 
> > But if you can find a nice way to make it work that'd be great.
> 
> How about using a list instead of an array ?
> This way we can add elements to this list when parsing the EDID.
> 
> Or we can just define a maximum size for the bus_formats array when
> retrieving this info from EDID. If I'm correct we have at most 18 bus
> formats:
>  - 3 color formats:
>    * RGB 4:4:4
>    * YCbCr 4:4:4
>    * YCbCr 4:4:2
>  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color

bpc isn't a bitmask, so EDID supports up to three formats only.

The color_formats field is computed in the drm_add_display_info() function. 
You could easily turn it into a local variable and allocate and fill the 
formats array at the end of the function.

On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote:
> Hi Boris,
> 
> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > >>
> > >> [snip]
> > >> 
> > >>>>>>> The new drm_display_info structure should look like this [2]
> > >>>>>>> (except that color_formats and bpc have not be removed yet), and
> > >>>>>>> [1] is just here to show how the video_bus_format enum would look
> > >>>>>>> like.
> > >>>>>>> 
> > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557
> > >>>>>>> [2] http://code.bulix.org/7n03b4-86556
> > >>>>>> 
> > >>>>>> Quoting from your paste:
> > >>>>>> 	+   const enum video_bus_format *bus_formats;
> > >>>>>> 	+   int nbus_formats;
> > >>>>>> 
> > >>>>>> Do we really need more than one?
> > >>>>> 
> > >>>>> We do if we want to replace the color_formats and bpc fields.
> > >>>> 
> > >>>> Yes, that's what I was about to answer :-).
> > >>> 
> > >>> Maybe we don't need to replace color_formats and bpc field
> > >>> immediately. That could be done in a follow-up patch.
> > >> 
> > >> We don't need to replace them right now, but we should at least agree on
> > >> how to replace them. Introducing a new field that would need to be
> > >> replaced in the near future when removing color_formats and bpc would
> > >> be a waste of time.
> > >
> > > Sure. One of the problems I see with replacing color_formats and bpc
> > > with the above is that some of the bits within color_formats are set
> > > when the EDID is parsed. That implies that if they are replaced with
> > > an array of formats, the array would need to be reallocated during EDID
> > > parsing. That sounds like ugliness.
> > > 
> > > But if you can find a nice way to make it work that'd be great.
> > 
> > How about using a list instead of an array ?
> > This way we can add elements to this list when parsing the EDID.
> > 
> > Or we can just define a maximum size for the bus_formats array when
> > retrieving this info from EDID. If I'm correct we have at most 18 bus
> > formats:
> >  - 3 color formats:
> >    * RGB 4:4:4
> >    * YCbCr 4:4:4
> >    * YCbCr 4:4:2
> >  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> 
> bpc isn't a bitmask, so EDID supports up to three formats only.
> 
> The color_formats field is computed in the drm_add_display_info() function. 
> You could easily turn it into a local variable and allocate and fill the 
> formats array at the end of the function.

But you also need to be careful to keep whatever formats the driver
might have set explicitly.

Thierry

On Mon, 21 Jul 2014 15:47:52 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Boris,
> 
> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > >>
> > >> [snip]
> > >> 
> > >>>>>>> The new drm_display_info structure should look like this [2]
> > >>>>>>> (except that color_formats and bpc have not be removed yet), and
> > >>>>>>> [1] is just here to show how the video_bus_format enum would look
> > >>>>>>> like.
> > >>>>>>> 
> > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557
> > >>>>>>> [2] http://code.bulix.org/7n03b4-86556
> > >>>>>> 
> > >>>>>> Quoting from your paste:
> > >>>>>> 	+   const enum video_bus_format *bus_formats;
> > >>>>>> 	+   int nbus_formats;
> > >>>>>> 
> > >>>>>> Do we really need more than one?
> > >>>>> 
> > >>>>> We do if we want to replace the color_formats and bpc fields.
> > >>>> 
> > >>>> Yes, that's what I was about to answer :-).
> > >>> 
> > >>> Maybe we don't need to replace color_formats and bpc field
> > >>> immediately. That could be done in a follow-up patch.
> > >> 
> > >> We don't need to replace them right now, but we should at least agree on
> > >> how to replace them. Introducing a new field that would need to be
> > >> replaced in the near future when removing color_formats and bpc would
> > >> be a waste of time.
> > >
> > > Sure. One of the problems I see with replacing color_formats and bpc
> > > with the above is that some of the bits within color_formats are set
> > > when the EDID is parsed. That implies that if they are replaced with
> > > an array of formats, the array would need to be reallocated during EDID
> > > parsing. That sounds like ugliness.
> > > 
> > > But if you can find a nice way to make it work that'd be great.
> > 
> > How about using a list instead of an array ?
> > This way we can add elements to this list when parsing the EDID.
> > 
> > Or we can just define a maximum size for the bus_formats array when
> > retrieving this info from EDID. If I'm correct we have at most 18 bus
> > formats:
> >  - 3 color formats:
> >    * RGB 4:4:4
> >    * YCbCr 4:4:4
> >    * YCbCr 4:4:2
> >  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> 
> bpc isn't a bitmask, so EDID supports up to three formats only.

Yes, bpc only contains a single value for now, and it fits the
DRM_EDID_DIGITAL_DEPTH field [1] (an enum defining the supported pixel
depth).
ITOH, DRM_EDID_HDMI_DC_XX [2] (which are referenced in the new
drm_assign_hdmi_deep_color_info function) are just bitmasks and thus a
display might support several color depth.

As a result, I wonder if we shouldn't start supporting several
color depths (as we do for color formats).

[1]http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_edid.c#L3436
[2]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_edid.c?id=refs/tags/v3.16-rc6#n3440

On Mon, 21 Jul 2014 15:54:12 +0200
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote:
> > Hi Boris,
> > 
> > On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> > > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> > > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> > > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> > > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> > > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> > > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> > > >>
> > > >> [snip]
> > > >> 
> > > >>>>>>> The new drm_display_info structure should look like this [2]
> > > >>>>>>> (except that color_formats and bpc have not be removed yet), and
> > > >>>>>>> [1] is just here to show how the video_bus_format enum would look
> > > >>>>>>> like.
> > > >>>>>>> 
> > > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557
> > > >>>>>>> [2] http://code.bulix.org/7n03b4-86556
> > > >>>>>> 
> > > >>>>>> Quoting from your paste:
> > > >>>>>> 	+   const enum video_bus_format *bus_formats;
> > > >>>>>> 	+   int nbus_formats;
> > > >>>>>> 
> > > >>>>>> Do we really need more than one?
> > > >>>>> 
> > > >>>>> We do if we want to replace the color_formats and bpc fields.
> > > >>>> 
> > > >>>> Yes, that's what I was about to answer :-).
> > > >>> 
> > > >>> Maybe we don't need to replace color_formats and bpc field
> > > >>> immediately. That could be done in a follow-up patch.
> > > >> 
> > > >> We don't need to replace them right now, but we should at least agree on
> > > >> how to replace them. Introducing a new field that would need to be
> > > >> replaced in the near future when removing color_formats and bpc would
> > > >> be a waste of time.
> > > >
> > > > Sure. One of the problems I see with replacing color_formats and bpc
> > > > with the above is that some of the bits within color_formats are set
> > > > when the EDID is parsed. That implies that if they are replaced with
> > > > an array of formats, the array would need to be reallocated during EDID
> > > > parsing. That sounds like ugliness.
> > > > 
> > > > But if you can find a nice way to make it work that'd be great.
> > > 
> > > How about using a list instead of an array ?
> > > This way we can add elements to this list when parsing the EDID.
> > > 
> > > Or we can just define a maximum size for the bus_formats array when
> > > retrieving this info from EDID. If I'm correct we have at most 18 bus
> > > formats:
> > >  - 3 color formats:
> > >    * RGB 4:4:4
> > >    * YCbCr 4:4:4
> > >    * YCbCr 4:4:2
> > >  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> > 
> > bpc isn't a bitmask, so EDID supports up to three formats only.
> > 
> > The color_formats field is computed in the drm_add_display_info() function. 
> > You could easily turn it into a local variable and allocate and fill the 
> > formats array at the end of the function.
> 
> But you also need to be careful to keep whatever formats the driver
> might have set explicitly.

Okay, in this case, using a list is a better idea, don't you think ?

On Mon, Jul 21, 2014 at 03:43:13PM +0200, Boris BREZILLON wrote:
> How about using a list instead of an array ?
> This way we can add elements to this list when parsing the EDID.
> 
> Or we can just define a maximum size for the bus_formats array when
> retrieving this info from EDID. If I'm correct we have at most 18 bus
> formats:
>  - 3 color formats:
>    * RGB 4:4:4
>    * YCbCr 4:4:4
>    * YCbCr 4:4:2
>  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color

This starts to worry me.  What are we trying to do here - are we trying
to encode the connection between the CRTC and the encoder, the encoder
and the connector, or the connector and the device?

The encoder to connector and connector to device is mostly a function of
the interface spec itself (for example, many HDMI encoders take either a
RGB or YUV input and can convert it to the HDMI specified colourspaces for
transmission over the connector.)

If you want to do encoder to connector, what about VGA or some other
analogue signalling such as TV composite?  It's easy to take this too
far...

Surely the only one which matters is the CRTC to the encoder - that's
certainly true of all the setups I've come across so far.  As for that
interface, CRTCs I've seen can produce a /wide/ range of different
representations.

Some CRTCs (eg, AMBA CLCD) produce R, G, B signals scrunched down on to
the LSB bits of a LCD data bus (so RGB888 uses 24 bits, RGB444 would
use the LSB 12 bits of those 24 - rather than outputting the R4 bits on
a subset of the R8 bits.)

What about RGB565 - where you have differing number of bits for the
green channel from red/blue?

Then you have red/blue colour swapping at the CRTC (and similar for YUV)
such as on Dove / Armada.

Then there are some encoders have the ability to almost arbitarily map
their input pins according to whatever you choose (eg, TDA998x).

This problem isn't as quite as simple as "this is what EDID gives us"
and "these are the number of bits representing a colour".

Hi Boris and Thierry,

On Monday 21 July 2014 16:21:36 Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 15:54:12 +0200 Thierry Reding wrote:
> > On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote:
> >> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> >>> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> >>>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> >>>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> >>>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> >>>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> >>>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:

[snip]

> >>>>>>>>> Quoting from your paste:
> >>>>>>>>> 	+   const enum video_bus_format *bus_formats;
> >>>>>>>>> 	+   int nbus_formats;
> >>>>>>>>> 
> >>>>>>>>> Do we really need more than one?
> >>>>>>>> 
> >>>>>>>> We do if we want to replace the color_formats and bpc fields.
> >>>>>>> 
> >>>>>>> Yes, that's what I was about to answer :-).
> >>>>>> 
> >>>>>> Maybe we don't need to replace color_formats and bpc field
> >>>>>> immediately. That could be done in a follow-up patch.
> >>>>> 
> >>>>> We don't need to replace them right now, but we should at least
> >>>>> agree on how to replace them. Introducing a new field that would
> >>>>> need to be replaced in the near future when removing color_formats
> >>>>> and bpc would be a waste of time.
> >>>> 
> >>>> Sure. One of the problems I see with replacing color_formats and bpc
> >>>> with the above is that some of the bits within color_formats are set
> >>>> when the EDID is parsed. That implies that if they are replaced with
> >>>> an array of formats, the array would need to be reallocated during
> >>>> EDID parsing. That sounds like ugliness.
> >>>> 
> >>>> But if you can find a nice way to make it work that'd be great.
> >>> 
> >>> How about using a list instead of an array ?
> >>> This way we can add elements to this list when parsing the EDID.
> >>> 
> >>> Or we can just define a maximum size for the bus_formats array when
> >>> retrieving this info from EDID. If I'm correct we have at most 18 bus
> >>> 
> >>> formats:
> >>>  - 3 color formats:
> >>>    * RGB 4:4:4
> >>>    * YCbCr 4:4:4
> >>>    * YCbCr 4:4:2
> >>>  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> >> 
> >> bpc isn't a bitmask, so EDID supports up to three formats only.
> >> 
> >> The color_formats field is computed in the drm_add_display_info()
> >> function. You could easily turn it into a local variable and allocate
> >> and fill the formats array at the end of the function.
> > 
> > But you also need to be careful to keep whatever formats the driver might
> > have set explicitly.

Do we have drivers that explicitly add formats to the formats parsed from EDID 
data ? If so, what's the use case ?

> Okay, in this case, using a list is a better idea, don't you think ?

I'd prefer an array if possible, as that would be easier to use for drivers.

In any case, we need to define who allocates and frees the array or the list 
elements, how and when.

Hi Boris,

On Monday 21 July 2014 16:18:10 Boris BREZILLON wrote:
> On Mon, 21 Jul 2014 15:47:52 +0200 Laurent Pinchart wrote:
> > On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> >> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> >>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> >>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> >>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> >>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> >>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> >>>>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote:
> >>>> [snip]
> >>>> 
> >>>>>>>>> The new drm_display_info structure should look like this [2]
> >>>>>>>>> (except that color_formats and bpc have not be removed yet), and
> >>>>>>>>> [1] is just here to show how the video_bus_format enum would
> >>>>>>>>> look like.
> >>>>>>>>> 
> >>>>>>>>> [1] http://code.bulix.org/rfd0yx-86557
> >>>>>>>>> [2] http://code.bulix.org/7n03b4-86556
> >>>>>>>> 
> >>>>>>>> Quoting from your paste:
> >>>>>>>> 	+   const enum video_bus_format *bus_formats;
> >>>>>>>> 	+   int nbus_formats;
> >>>>>>>> 
> >>>>>>>> Do we really need more than one?
> >>>>>>> 
> >>>>>>> We do if we want to replace the color_formats and bpc fields.
> >>>>>> 
> >>>>>> Yes, that's what I was about to answer :-).
> >>>>> 
> >>>>> Maybe we don't need to replace color_formats and bpc field
> >>>>> immediately. That could be done in a follow-up patch.
> >>>> 
> >>>> We don't need to replace them right now, but we should at least agree
> >>>> on how to replace them. Introducing a new field that would need to be
> >>>> replaced in the near future when removing color_formats and bpc would
> >>>> be a waste of time.
> >>> 
> >>> Sure. One of the problems I see with replacing color_formats and bpc
> >>> with the above is that some of the bits within color_formats are set
> >>> when the EDID is parsed. That implies that if they are replaced with
> >>> an array of formats, the array would need to be reallocated during
> >>> EDID parsing. That sounds like ugliness.
> >>> 
> >>> But if you can find a nice way to make it work that'd be great.
> >> 
> >> How about using a list instead of an array ?
> >> This way we can add elements to this list when parsing the EDID.
> >> 
> >> Or we can just define a maximum size for the bus_formats array when
> >> retrieving this info from EDID. If I'm correct we have at most 18 bus
> >> 
> >> formats:
> >>  - 3 color formats:
> >>    * RGB 4:4:4
> >>    * YCbCr 4:4:4
> >>    * YCbCr 4:4:2
> >>  
> >>  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> > 
> > bpc isn't a bitmask, so EDID supports up to three formats only.
> 
> Yes, bpc only contains a single value for now, and it fits the
> DRM_EDID_DIGITAL_DEPTH field [1] (an enum defining the supported pixel
> depth).
> ITOH, DRM_EDID_HDMI_DC_XX [2] (which are referenced in the new
> drm_assign_hdmi_deep_color_info function) are just bitmasks and thus a
> display might support several color depth.
> 
> As a result, I wonder if we shouldn't start supporting several color depths
> (as we do for color formats)

If there's a use case for that, sure. It wouldn't be difficult, given that a 
bus format defines both the color format and the depths.

> [1]http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_edid.c#L3436
> [2]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/driv
> ers/gpu/drm/drm_edid.c?id=refs/tags/v3.16-rc6#n3440

On Mon, Jul 21, 2014 at 08:30:31PM +0200, Laurent Pinchart wrote:
> Hi Boris and Thierry,
> 
> On Monday 21 July 2014 16:21:36 Boris BREZILLON wrote:
> > On Mon, 21 Jul 2014 15:54:12 +0200 Thierry Reding wrote:
> > > On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote:
> > >> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote:
> > >>> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote:
> > >>>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote:
> > >>>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote:
> > >>>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote:
> > >>>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote:
> > >>>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote:
> 
> [snip]
> 
> > >>>>>>>>> Quoting from your paste:
> > >>>>>>>>> 	+   const enum video_bus_format *bus_formats;
> > >>>>>>>>> 	+   int nbus_formats;
> > >>>>>>>>> 
> > >>>>>>>>> Do we really need more than one?
> > >>>>>>>> 
> > >>>>>>>> We do if we want to replace the color_formats and bpc fields.
> > >>>>>>> 
> > >>>>>>> Yes, that's what I was about to answer :-).
> > >>>>>> 
> > >>>>>> Maybe we don't need to replace color_formats and bpc field
> > >>>>>> immediately. That could be done in a follow-up patch.
> > >>>>> 
> > >>>>> We don't need to replace them right now, but we should at least
> > >>>>> agree on how to replace them. Introducing a new field that would
> > >>>>> need to be replaced in the near future when removing color_formats
> > >>>>> and bpc would be a waste of time.
> > >>>> 
> > >>>> Sure. One of the problems I see with replacing color_formats and bpc
> > >>>> with the above is that some of the bits within color_formats are set
> > >>>> when the EDID is parsed. That implies that if they are replaced with
> > >>>> an array of formats, the array would need to be reallocated during
> > >>>> EDID parsing. That sounds like ugliness.
> > >>>> 
> > >>>> But if you can find a nice way to make it work that'd be great.
> > >>> 
> > >>> How about using a list instead of an array ?
> > >>> This way we can add elements to this list when parsing the EDID.
> > >>> 
> > >>> Or we can just define a maximum size for the bus_formats array when
> > >>> retrieving this info from EDID. If I'm correct we have at most 18 bus
> > >>> 
> > >>> formats:
> > >>>  - 3 color formats:
> > >>>    * RGB 4:4:4
> > >>>    * YCbCr 4:4:4
> > >>>    * YCbCr 4:4:2
> > >>>  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> > >> 
> > >> bpc isn't a bitmask, so EDID supports up to three formats only.
> > >> 
> > >> The color_formats field is computed in the drm_add_display_info()
> > >> function. You could easily turn it into a local variable and allocate
> > >> and fill the formats array at the end of the function.
> > > 
> > > But you also need to be careful to keep whatever formats the driver might
> > > have set explicitly.
> 
> Do we have drivers that explicitly add formats to the formats parsed from EDID 
> data ? If so, what's the use case ?

Drivers could specifically add them if there's no EDID or if the EDID
is known to be broken. If the former this is probably irrelevant. In the
latter maybe a better option would be to ignore the EDID-probed ones
rather than use the union of those provided by the driver and EDID.

Thierry

On Mon, Jul 21, 2014 at 06:06:30PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 21, 2014 at 03:43:13PM +0200, Boris BREZILLON wrote:
> > How about using a list instead of an array ?
> > This way we can add elements to this list when parsing the EDID.
> > 
> > Or we can just define a maximum size for the bus_formats array when
> > retrieving this info from EDID. If I'm correct we have at most 18 bus
> > formats:
> >  - 3 color formats:
> >    * RGB 4:4:4
> >    * YCbCr 4:4:4
> >    * YCbCr 4:4:2
> >  - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
> 
> This starts to worry me.  What are we trying to do here - are we trying
> to encode the connection between the CRTC and the encoder, the encoder
> and the connector, or the connector and the device?

This is about the bus format of the panel device. That would make it the
latter.

> The encoder to connector and connector to device is mostly a function of
> the interface spec itself (for example, many HDMI encoders take either a
> RGB or YUV input and can convert it to the HDMI specified colourspaces for
> transmission over the connector.)

The discussion here started because we currently have no way to store
information about the interface for raw RGB. That means currently all
drivers need to hardcode assumptions about the mode. The idea was to
make this information available via a field in drm_display_info so that
drivers could reconfigure depending on what the attached panel expects.

This doesn't only apply to panels, though, the issue is the same when a
bridge (RGB/LVDS for example) is connected to the encoder.

> If you want to do encoder to connector, what about VGA or some other
> analogue signalling such as TV composite?  It's easy to take this too
> far...
> 
> Surely the only one which matters is the CRTC to the encoder - that's
> certainly true of all the setups I've come across so far.  As for that
> interface, CRTCs I've seen can produce a /wide/ range of different
> representations.
> 
> Some CRTCs (eg, AMBA CLCD) produce R, G, B signals scrunched down on to
> the LSB bits of a LCD data bus (so RGB888 uses 24 bits, RGB444 would
> use the LSB 12 bits of those 24 - rather than outputting the R4 bits on
> a subset of the R8 bits.)
> 
> What about RGB565 - where you have differing number of bits for the
> green channel from red/blue?
> 
> Then you have red/blue colour swapping at the CRTC (and similar for YUV)
> such as on Dove / Armada.
> 
> Then there are some encoders have the ability to almost arbitarily map
> their input pins according to whatever you choose (eg, TDA998x).
> 
> This problem isn't as quite as simple as "this is what EDID gives us"
> and "these are the number of bits representing a colour".

I think what we need is a way to pass information from whatever device
is behind the encoder (be it a bridge or a panel) to the encoder. And
likely we'll need a similar (or the same) way to pass that information
from bridge to bridge or from bridge to panel.

That way the encoder can ask the bridge (or panel) about the format it
requires and reconfigure itself accordingly. This should be able to work
with an arbitrary bridge -> [bridge... ->] panel chain where each
element in the chain can reconfigure depending on what the next element
requires (or fail if it can't work, which should really never happen).

Thierry