Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
On 2018-04-21 10:38, Laurent Pinchart wrote:
> Hi Peter,
>
> On Friday, 20 April 2018 15:55:50 EEST Peter Rosin wrote:
>> On 2018-04-20 13:38, jacopo mondi wrote:
>>> On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote:
On 2018-04-20 12:18, Laurent Pinchart wrote:
> On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
>> Hi Peter,
>>
>> I've been a bit a pain in the arse for you recently, but please
>> bear with me a bit more, and sorry for jumping late on the band wagon.
>>
>> On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
>>> Hi!
>>>
>>> I naively thought that since there was support for both nxp,tda19988
>>> (in the tda998x driver) and the atmel-hlcdc, things would be a smooth
>>> ride. But it wasn't, so I started looking around and realized I had to
>>> fix things.
>>>
>>> In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
>>> component, but now in v3 I fix things by making the tda998x driver
>>> a bridge instead. This was after a suggestion from Boris Brezillion
That should be Brezillon, sorry for being sloppy with the spelling.
>>> (the atmel-hlcdc maintainer), so there was some risk of bias ... but
>>> after comparing what was needed, I too find the bridge approach
>>> better.
>>>
>>> In addition to the above, our PCB interface between the SAMA5D3 and
>>> the HDMI encoder is only using 16 bits, and this has to be described
>>> somewhere, or the atmel-hlcdc driver have no chance of selecting the
>>> correct output mode. Since I have similar problems with a ds90c185
>>> lvds encoder I added patches to override the atmel-hlcdc output format
>>> via DT properties compatible with the media video-interface binding
>>> and things start to play together.
>>>
>>> Since this series superseeds the bridge series [1], I have included
>>> the leftover bindings patch for the ti,ds90c185 here.
>>
>> I feel like this series would look better if it would make use of the
>> proposed bridge media bus format support I have recently sent out [1]
>> (and which was not there when you first sent v1).
>>
>> I understand your fundamental problem here is that the bus format
>> that should be reported by your bridge is different from the ones
>> actually supported by the TDA19988 chip, as the wirings ground some
>> of the input pins.
>>
>> Although this is defintely something that could be described in the
>> bridge's own OF node with the 'bus_width' property, and what I would
>> do, now that you have made a bridge out from the tda19988 driver, is:
>>
>> 1) Set the bridge accepted input bus_format parsing its pin
>> configuration, or default it if that's not implemented yet.
>> This will likely be rgb888. You can do that using the trivial
>> support for bridge input image formats implemented by my series.
>> 2) Specify in the bridge endpoint a 'bus_width' of <16>
>> 3) In your atmel-hlcd get both the image format of the bridge (rgb888)
>> and parse the remote endpoint property 'bus_width' and get the <16>
>> value back.
>
> Parsing properties of remote nodes should be avoided as much as
> possible, as they need to be interpreted in the context of the DT
> bindings related to the compatible string applicable to that node. I'd
> rather have the bus_width property in the local endpoint node.
In addition to that, my view of this binding
endpoint {
bus-type = <0>;
>>>
>>> bus-type is used by v4l2_fwnode helpers to decide which type of bus
>>> they're dealing with iirc. Here it seems to me it has no added
>>> value, also because in your bindings description "it shall be 0" and
>>> it is not parsed anywhere in you code, but no big deal
>>
>> bus-type is indeed parsed and verified to be zero which means auto-detect
>> according to the video-interfaces binding. An auto-detect bus-type with
>> a given bus-width means a parallel interface IIUC.
>
> I believe you could leave it out though. Your display controller only
> supports
> parallel buses, so there's no need to specify the bus type explicitly.
You need it if you want to verify that you conform to the video-interfaces
binding. And that verification is only needed *if* we want to build upon
the drm_of_media_bus_fmt function so that it eventually support other formats.
Where I'm coming from is this discussion with Rob:
https://lkml.org/lkml/2018/4/9/286
I interpreted that as is seen in patch 2/7 and 3/7 in v3 (and v2). True,
I might have read too much into his desire to have something generic,
and that the generic bit only applied to the binding, but 2/7 and 3/7
was how I interpreted that conversation.
>> From patch 3/7:
>>
>> if (of_property_read_u32(node, "bus-type", _type))
>> return 0;
>> if (bus_type != 0)
>>
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
On 2018-04-20 13:38, jacopo mondi wrote:
> Hi Peter,
>
> On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote:
>> On 2018-04-20 12:18, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
Hi Peter,
I've been a bit a pain in the arse for you recently, but please
bear with me a bit more, and sorry for jumping late on the band wagon.
On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
> Hi!
>
> I naively thought that since there was support for both nxp,tda19988 (in
> the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
> But it wasn't, so I started looking around and realized I had to fix
> things.
>
> In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
> component, but now in v3 I fix things by making the tda998x driver
> a bridge instead. This was after a suggestion from Boris Brezillion
>>
>> That should be Brezillon, sorry for being sloppy with the spelling.
>>
> (the atmel-hlcdc maintainer), so there was some risk of bias ... but
> after comparing what was needed, I too find the bridge approach better.
>
> In addition to the above, our PCB interface between the SAMA5D3 and the
> HDMI encoder is only using 16 bits, and this has to be described
> somewhere, or the atmel-hlcdc driver have no chance of selecting the
> correct output mode. Since I have similar problems with a ds90c185 lvds
> encoder I added patches to override the atmel-hlcdc output format via
> DT properties compatible with the media video-interface binding and
> things start to play together.
>
> Since this series superseeds the bridge series [1], I have included the
> leftover bindings patch for the ti,ds90c185 here.
I feel like this series would look better if it would make use of the
proposed bridge media bus format support I have recently sent out [1]
(and which was not there when you first sent v1).
I understand your fundamental problem here is that the bus format
that should be reported by your bridge is different from the ones
actually supported by the TDA19988 chip, as the wirings ground some
of the input pins.
Although this is defintely something that could be described in the
bridge's own OF node with the 'bus_width' property, and what I would do,
now that you have made a bridge out from the tda19988 driver, is:
1) Set the bridge accepted input bus_format parsing its pin
configuration, or default it if that's not implemented yet.
This will likely be rgb888. You can do that using the trivial
support for bridge input image formats implemented by my series.
2) Specify in the bridge endpoint a 'bus_width' of <16>
3) In your atmel-hlcd get both the image format of the bridge (rgb888)
and parse the remote endpoint property 'bus_width' and get the <16>
value back.
>>>
>>> Parsing properties of remote nodes should be avoided as much as possible, as
>>> they need to be interpreted in the context of the DT bindings related to the
>>> compatible string applicable to that node. I'd rather have the bus_width
>>> property in the local endpoint node.
>>
>> In addition to that, my view of this binding
>>
>> endpoint {
>> bus-type = <0>;
>
> bus-type is used by v4l2_fwnode helpers to decide which type of bus
> they're dealing with iirc. Here it seems to me it has no added
> value, also because in your bindings description "it shall be 0" and
> it is not parsed anywhere in you code, but no big deal
bus-type is indeed parsed and verified to be zero which means auto-detect
according to the video-interfaces binding. An auto-detect bus-type with
a given bus-width means a parallel interface IIUC.
From patch 3/7:
if (of_property_read_u32(node, "bus-type", _type))
return 0;
if (bus_type != 0)
return -EINVAL;
>> bus-widht = <16>;
>> };
>>
>> is that it always means rgb565. See further below.
>>
4) Set the correct format in the atmel hlcd driver to accommodate a
bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565,
or are there other possible combinations I am missing?)
I would consider this better mostly because in this series you are
creating a semantic for the whole DRM subsystem on the 'bus_width'
property, where numerical values as '12', '16' etc are arbitrary tied
to the selection of a media bus format. At least you should use a
common set of defines [1] between the device tree and the driver,
but this looks anyway fragile imho.
>>>
>>> This I agree with though. Combining the remote bus format with the local bus
>>> width should fix the problem without having to parse remote properties.
>>
>> My thinking was that the binding with bus-type = <0> and bus-width =
>>
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
On 2018-04-20 12:18, Laurent Pinchart wrote:
> Hello,
>
> On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
>> Hi Peter,
>> I've been a bit a pain in the arse for you recently, but please
>> bear with me a bit more, and sorry for jumping late on the band wagon.
>>
>> On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
>>> Hi!
>>>
>>> I naively thought that since there was support for both nxp,tda19988 (in
>>> the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
>>> But it wasn't, so I started looking around and realized I had to fix
>>> things.
>>>
>>> In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
>>> component, but now in v3 I fix things by making the tda998x driver
>>> a bridge instead. This was after a suggestion from Boris Brezillion
That should be Brezillon, sorry for being sloppy with the spelling.
>>> (the atmel-hlcdc maintainer), so there was some risk of bias ... but
>>> after comparing what was needed, I too find the bridge approach better.
>>>
>>> In addition to the above, our PCB interface between the SAMA5D3 and the
>>> HDMI encoder is only using 16 bits, and this has to be described
>>> somewhere, or the atmel-hlcdc driver have no chance of selecting the
>>> correct output mode. Since I have similar problems with a ds90c185 lvds
>>> encoder I added patches to override the atmel-hlcdc output format via
>>> DT properties compatible with the media video-interface binding and
>>> things start to play together.
>>>
>>> Since this series superseeds the bridge series [1], I have included the
>>> leftover bindings patch for the ti,ds90c185 here.
>>
>> I feel like this series would look better if it would make use of the
>> proposed bridge media bus format support I have recently sent out [1]
>> (and which was not there when you first sent v1).
>>
>> I understand your fundamental problem here is that the bus format
>> that should be reported by your bridge is different from the ones
>> actually supported by the TDA19988 chip, as the wirings ground some
>> of the input pins.
>>
>> Although this is defintely something that could be described in the
>> bridge's own OF node with the 'bus_width' property, and what I would do,
>> now that you have made a bridge out from the tda19988 driver, is:
>>
>> 1) Set the bridge accepted input bus_format parsing its pin
>> configuration, or default it if that's not implemented yet.
>> This will likely be rgb888. You can do that using the trivial
>> support for bridge input image formats implemented by my series.
>> 2) Specify in the bridge endpoint a 'bus_width' of <16>
>> 3) In your atmel-hlcd get both the image format of the bridge (rgb888)
>> and parse the remote endpoint property 'bus_width' and get the <16>
>> value back.
>
> Parsing properties of remote nodes should be avoided as much as possible, as
> they need to be interpreted in the context of the DT bindings related to the
> compatible string applicable to that node. I'd rather have the bus_width
> property in the local endpoint node.
In addition to that, my view of this binding
endpoint {
bus-type = <0>;
bus-widht = <16>;
};
is that it always means rgb565. See further below.
>> 4) Set the correct format in the atmel hlcd driver to accommodate a
>> bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565,
>> or are there other possible combinations I am missing?)
>>
>> I would consider this better mostly because in this series you are
>> creating a semantic for the whole DRM subsystem on the 'bus_width'
>> property, where numerical values as '12', '16' etc are arbitrary tied
>> to the selection of a media bus format. At least you should use a
>> common set of defines [1] between the device tree and the driver,
>> but this looks anyway fragile imho.
>
> This I agree with though. Combining the remote bus format with the local bus
> width should fix the problem without having to parse remote properties.
My thinking was that the binding with bus-type = <0> and bus-width =
would mean a parallel bus (type 0 means auto-detect and with a bus-width that
auto-detect means a parallel bus) and the most natural/common interpretation
of that bus-width. For bus widths of 12, 16, 18, 24, 30 etc I think that
would be rgb444, rgb565, rgb666, rgb888, rgb101010 (or, I'm first so I get
to define the default). If you have some other interpretation of a bus with
that width, you'd need to extend the video-interface binding with some way
of saying what you need, perhaps using some kind of data mapping or something
to say e.g. bgr666. And you'd need some kind of indicator if you have YUV
signals instead of RGB, and LVDS isn't a completely parallel bus, so you'd
need something for that. Etc.
Because the word from Rob was that there should be one common binding that
describes video interfaces. I started an implementation that interprets that
binding in a drm context in
[PATCH 3/7] drm: of:
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hi Peter,
On Friday, 20 April 2018 15:55:50 EEST Peter Rosin wrote:
> On 2018-04-20 13:38, jacopo mondi wrote:
> > On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote:
> >> On 2018-04-20 12:18, Laurent Pinchart wrote:
> >>> On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
> Hi Peter,
>
> I've been a bit a pain in the arse for you recently, but please
> bear with me a bit more, and sorry for jumping late on the band wagon.
>
> On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
> > Hi!
> >
> > I naively thought that since there was support for both nxp,tda19988
> > (in the tda998x driver) and the atmel-hlcdc, things would be a smooth
> > ride. But it wasn't, so I started looking around and realized I had to
> > fix things.
> >
> > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
> > component, but now in v3 I fix things by making the tda998x driver
> > a bridge instead. This was after a suggestion from Boris Brezillion
> >>
> >> That should be Brezillon, sorry for being sloppy with the spelling.
> >>
> > (the atmel-hlcdc maintainer), so there was some risk of bias ... but
> > after comparing what was needed, I too find the bridge approach
> > better.
> >
> > In addition to the above, our PCB interface between the SAMA5D3 and
> > the HDMI encoder is only using 16 bits, and this has to be described
> > somewhere, or the atmel-hlcdc driver have no chance of selecting the
> > correct output mode. Since I have similar problems with a ds90c185
> > lvds encoder I added patches to override the atmel-hlcdc output format
> > via DT properties compatible with the media video-interface binding
> > and things start to play together.
> >
> > Since this series superseeds the bridge series [1], I have included
> > the leftover bindings patch for the ti,ds90c185 here.
>
> I feel like this series would look better if it would make use of the
> proposed bridge media bus format support I have recently sent out [1]
> (and which was not there when you first sent v1).
>
> I understand your fundamental problem here is that the bus format
> that should be reported by your bridge is different from the ones
> actually supported by the TDA19988 chip, as the wirings ground some
> of the input pins.
>
> Although this is defintely something that could be described in the
> bridge's own OF node with the 'bus_width' property, and what I would
> do, now that you have made a bridge out from the tda19988 driver, is:
>
> 1) Set the bridge accepted input bus_format parsing its pin
> configuration, or default it if that's not implemented yet.
> This will likely be rgb888. You can do that using the trivial
> support for bridge input image formats implemented by my series.
> 2) Specify in the bridge endpoint a 'bus_width' of <16>
> 3) In your atmel-hlcd get both the image format of the bridge (rgb888)
> and parse the remote endpoint property 'bus_width' and get the <16>
> value back.
> >>>
> >>> Parsing properties of remote nodes should be avoided as much as
> >>> possible, as they need to be interpreted in the context of the DT
> >>> bindings related to the compatible string applicable to that node. I'd
> >>> rather have the bus_width property in the local endpoint node.
> >>
> >> In addition to that, my view of this binding
> >>
> >>endpoint {
> >>bus-type = <0>;
> >
> > bus-type is used by v4l2_fwnode helpers to decide which type of bus
> > they're dealing with iirc. Here it seems to me it has no added
> > value, also because in your bindings description "it shall be 0" and
> > it is not parsed anywhere in you code, but no big deal
>
> bus-type is indeed parsed and verified to be zero which means auto-detect
> according to the video-interfaces binding. An auto-detect bus-type with
> a given bus-width means a parallel interface IIUC.
I believe you could leave it out though. Your display controller only supports
parallel buses, so there's no need to specify the bus type explicitly.
> From patch 3/7:
>
> if (of_property_read_u32(node, "bus-type", _type))
> return 0;
> if (bus_type != 0)
> return -EINVAL;
>
> >>bus-widht = <16>;
> >>};
> >>
> >> is that it always means rgb565. See further below.
> >>
> 4) Set the correct format in the atmel hlcd driver to accommodate a
> bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565,
> or are there other possible combinations I am missing?)
>
> I would consider this better mostly because in this series you are
> creating a semantic for the whole DRM subsystem on the 'bus_width'
> property, where numerical values as '12', '16' etc are arbitrary tied
> to the selection of a
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hi Jacopo, On Friday, 20 April 2018 14:22:04 EEST jacopo mondi wrote: > On Fri, Apr 20, 2018 at 01:18:13PM +0300, Laurent Pinchart wrote: > > On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > > > Hi Peter, > > > > > > I've been a bit a pain in the arse for you recently, but please > > > bear with me a bit more, and sorry for jumping late on the band wagon. > > > > > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > > > Hi! > > > > > > > > I naively thought that since there was support for both nxp,tda19988 > > > > (in the tda998x driver) and the atmel-hlcdc, things would be a smooth > > > > ride. But it wasn't, so I started looking around and realized I had to > > > > fix things. > > > > > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > > > component, but now in v3 I fix things by making the tda998x driver > > > > a bridge instead. This was after a suggestion from Boris Brezillion > > > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > > > after comparing what was needed, I too find the bridge approach > > > > better. > > > > > > > > In addition to the above, our PCB interface between the SAMA5D3 and > > > > the HDMI encoder is only using 16 bits, and this has to be described > > > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > > > correct output mode. Since I have similar problems with a ds90c185 > > > > lvds encoder I added patches to override the atmel-hlcdc output format > > > > via DT properties compatible with the media video-interface binding > > > > and things start to play together. > > > > > > > > Since this series superseeds the bridge series [1], I have included > > > > the leftover bindings patch for the ti,ds90c185 here. > > > > > > I feel like this series would look better if it would make use of the > > > proposed bridge media bus format support I have recently sent out [1] > > > (and which was not there when you first sent v1). > > > > > > I understand your fundamental problem here is that the bus format > > > that should be reported by your bridge is different from the ones > > > actually supported by the TDA19988 chip, as the wirings ground some > > > of the input pins. > > > > > > Although this is defintely something that could be described in the > > > bridge's own OF node with the 'bus_width' property, and what I would do, > > > now that you have made a bridge out from the tda19988 driver, is: > > > > > > 1) Set the bridge accepted input bus_format parsing its pin > > > configuration, or default it if that's not implemented yet. > > > This will likely be rgb888. You can do that using the trivial > > > support for bridge input image formats implemented by my series. > > > 2) Specify in the bridge endpoint a 'bus_width' of <16> > > > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > > > and parse the remote endpoint property 'bus_width' and get the <16> > > > value back. > > > > Parsing properties of remote nodes should be avoided as much as possible, > > as they need to be interpreted in the context of the DT bindings related > > to the compatible string applicable to that node. I'd rather have the > > bus_width property in the local endpoint node. > > Right, I see... > Well, in Peter's case, the bus width, being a consequence of > wirings, can be described safely in both endpoints, right? If we look at it from the endpoints' point of view, the display controller has to output a 16-bit format, and the HDMI encoder receives a 24-bit format. The conversion is due to PCB wiring so there's no DT node to describe that. I thus believe the right description to be bus-width = <16> on the display controller side, and bus-width = <24> on the HDMI encoder side. This being said, even if the bus-width property was set to 16 on both sides, what I frown upon is parsing a property of the HDMI encoder DT node in the display controller driver. > > > 4) Set the correct format in the atmel hlcd driver to accommodate a > > > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > > > or are there other possible combinations I am missing?) > > > > > > I would consider this better mostly because in this series you are > > > creating a semantic for the whole DRM subsystem on the 'bus_width' > > > property, where numerical values as '12', '16' etc are arbitrary tied > > > to the selection of a media bus format. At least you should use a > > > common set of defines [1] between the device tree and the driver, > > > but this looks anyway fragile imho. > > > > This I agree with though. Combining the remote bus format with the local > > bus width should fix the problem without having to parse remote > > properties. > > > > > Have I maybe missed some parts of the problem you are trying to solve > > > here? > > > > > > [1] drm: bridge: Add support for static image formats > > > > > > https://lwn.net/Articles/752296/ > > > > > > [2]
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hi Peter,
On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote:
> On 2018-04-20 12:18, Laurent Pinchart wrote:
> > Hello,
> >
> > On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote:
> >> Hi Peter,
> >> I've been a bit a pain in the arse for you recently, but please
> >> bear with me a bit more, and sorry for jumping late on the band wagon.
> >>
> >> On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote:
> >>> Hi!
> >>>
> >>> I naively thought that since there was support for both nxp,tda19988 (in
> >>> the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
> >>> But it wasn't, so I started looking around and realized I had to fix
> >>> things.
> >>>
> >>> In v1 and v2 I fixed things by making the atmel-hlcdc driver a master
> >>> component, but now in v3 I fix things by making the tda998x driver
> >>> a bridge instead. This was after a suggestion from Boris Brezillion
>
> That should be Brezillon, sorry for being sloppy with the spelling.
>
> >>> (the atmel-hlcdc maintainer), so there was some risk of bias ... but
> >>> after comparing what was needed, I too find the bridge approach better.
> >>>
> >>> In addition to the above, our PCB interface between the SAMA5D3 and the
> >>> HDMI encoder is only using 16 bits, and this has to be described
> >>> somewhere, or the atmel-hlcdc driver have no chance of selecting the
> >>> correct output mode. Since I have similar problems with a ds90c185 lvds
> >>> encoder I added patches to override the atmel-hlcdc output format via
> >>> DT properties compatible with the media video-interface binding and
> >>> things start to play together.
> >>>
> >>> Since this series superseeds the bridge series [1], I have included the
> >>> leftover bindings patch for the ti,ds90c185 here.
> >>
> >> I feel like this series would look better if it would make use of the
> >> proposed bridge media bus format support I have recently sent out [1]
> >> (and which was not there when you first sent v1).
> >>
> >> I understand your fundamental problem here is that the bus format
> >> that should be reported by your bridge is different from the ones
> >> actually supported by the TDA19988 chip, as the wirings ground some
> >> of the input pins.
> >>
> >> Although this is defintely something that could be described in the
> >> bridge's own OF node with the 'bus_width' property, and what I would do,
> >> now that you have made a bridge out from the tda19988 driver, is:
> >>
> >> 1) Set the bridge accepted input bus_format parsing its pin
> >> configuration, or default it if that's not implemented yet.
> >> This will likely be rgb888. You can do that using the trivial
> >> support for bridge input image formats implemented by my series.
> >> 2) Specify in the bridge endpoint a 'bus_width' of <16>
> >> 3) In your atmel-hlcd get both the image format of the bridge (rgb888)
> >> and parse the remote endpoint property 'bus_width' and get the <16>
> >> value back.
> >
> > Parsing properties of remote nodes should be avoided as much as possible, as
> > they need to be interpreted in the context of the DT bindings related to the
> > compatible string applicable to that node. I'd rather have the bus_width
> > property in the local endpoint node.
>
> In addition to that, my view of this binding
>
> endpoint {
> bus-type = <0>;
bus-type is used by v4l2_fwnode helpers to decide which type of bus
they're dealing with iirc. Here it seems to me it has no added
value, also because in your bindings description "it shall be 0" and
it is not parsed anywhere in you code, but no big deal
> bus-widht = <16>;
> };
>
> is that it always means rgb565. See further below.
>
> >> 4) Set the correct format in the atmel hlcd driver to accommodate a
> >> bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565,
> >> or are there other possible combinations I am missing?)
> >>
> >> I would consider this better mostly because in this series you are
> >> creating a semantic for the whole DRM subsystem on the 'bus_width'
> >> property, where numerical values as '12', '16' etc are arbitrary tied
> >> to the selection of a media bus format. At least you should use a
> >> common set of defines [1] between the device tree and the driver,
> >> but this looks anyway fragile imho.
> >
> > This I agree with though. Combining the remote bus format with the local bus
> > width should fix the problem without having to parse remote properties.
>
> My thinking was that the binding with bus-type = <0> and bus-width =
> would mean a parallel bus (type 0 means auto-detect and with a bus-width that
> auto-detect means a parallel bus) and the most natural/common interpretation
> of that bus-width. For bus widths of 12, 16, 18, 24, 30 etc I think that
> would be rgb444, rgb565, rgb666, rgb888, rgb101010 (or, I'm first so I get
> to define the default). If you have some other interpretation of a bus with
> that width, you'd need to extend the
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hi Laurent, On Fri, Apr 20, 2018 at 01:18:13PM +0300, Laurent Pinchart wrote: > Hello, > > On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > > Hi Peter, > > I've been a bit a pain in the arse for you recently, but please > > bear with me a bit more, and sorry for jumping late on the band wagon. > > > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > > Hi! > > > > > > I naively thought that since there was support for both nxp,tda19988 (in > > > the tda998x driver) and the atmel-hlcdc, things would be a smooth ride. > > > But it wasn't, so I started looking around and realized I had to fix > > > things. > > > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > > component, but now in v3 I fix things by making the tda998x driver > > > a bridge instead. This was after a suggestion from Boris Brezillion > > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > > after comparing what was needed, I too find the bridge approach better. > > > > > > In addition to the above, our PCB interface between the SAMA5D3 and the > > > HDMI encoder is only using 16 bits, and this has to be described > > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > > correct output mode. Since I have similar problems with a ds90c185 lvds > > > encoder I added patches to override the atmel-hlcdc output format via > > > DT properties compatible with the media video-interface binding and > > > things start to play together. > > > > > > Since this series superseeds the bridge series [1], I have included the > > > leftover bindings patch for the ti,ds90c185 here. > > > > I feel like this series would look better if it would make use of the > > proposed bridge media bus format support I have recently sent out [1] > > (and which was not there when you first sent v1). > > > > I understand your fundamental problem here is that the bus format > > that should be reported by your bridge is different from the ones > > actually supported by the TDA19988 chip, as the wirings ground some > > of the input pins. > > > > Although this is defintely something that could be described in the > > bridge's own OF node with the 'bus_width' property, and what I would do, > > now that you have made a bridge out from the tda19988 driver, is: > > > > 1) Set the bridge accepted input bus_format parsing its pin > > configuration, or default it if that's not implemented yet. > > This will likely be rgb888. You can do that using the trivial > > support for bridge input image formats implemented by my series. > > 2) Specify in the bridge endpoint a 'bus_width' of <16> > > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > > and parse the remote endpoint property 'bus_width' and get the <16> > > value back. > > Parsing properties of remote nodes should be avoided as much as possible, as > they need to be interpreted in the context of the DT bindings related to the > compatible string applicable to that node. I'd rather have the bus_width > property in the local endpoint node. Right, I see... Well, in Peter's case, the bus width, being a consequence of wirings, can be described safely in both endpoints, right? > > > 4) Set the correct format in the atmel hlcd driver to accommodate a > > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > > or are there other possible combinations I am missing?) > > > > I would consider this better mostly because in this series you are > > creating a semantic for the whole DRM subsystem on the 'bus_width' > > property, where numerical values as '12', '16' etc are arbitrary tied > > to the selection of a media bus format. At least you should use a > > common set of defines [1] between the device tree and the driver, > > but this looks anyway fragile imho. > > This I agree with though. Combining the remote bus format with the local bus > width should fix the problem without having to parse remote properties. > > > Have I maybe missed some parts of the problem you are trying to solve > > here? > > > > Thank you > >j > > > > [1] drm: bridge: Add support for static image formats > > https://lwn.net/Articles/752296/ > > [2] include/uapi/linux/media-bus-format.h > > > > > Anyway, this series solves some real issues for my HW. > > > > > > Cheers, > > > Peter > > > > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > > > - patch 2/7 fixed spelling and added an example > > > - patch 4/7 parse the DT up front and store the result indexed by encoder > > > - old patch 5/6 and 6/6 dropped > > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > > > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > > > - added reviewed-by from Rob to patch 1/6 > > > - patch 2/6 changed so that the bus format override is in the endpoint > > > DT node, and follows the binding of media video-interfaces. > > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > > >
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hello, On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > Hi Peter, > I've been a bit a pain in the arse for you recently, but please > bear with me a bit more, and sorry for jumping late on the band wagon. > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > Hi! > > > > I naively thought that since there was support for both nxp,tda19988 (in > > the tda998x driver) and the atmel-hlcdc, things would be a smooth ride. > > But it wasn't, so I started looking around and realized I had to fix > > things. > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > component, but now in v3 I fix things by making the tda998x driver > > a bridge instead. This was after a suggestion from Boris Brezillion > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > after comparing what was needed, I too find the bridge approach better. > > > > In addition to the above, our PCB interface between the SAMA5D3 and the > > HDMI encoder is only using 16 bits, and this has to be described > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > correct output mode. Since I have similar problems with a ds90c185 lvds > > encoder I added patches to override the atmel-hlcdc output format via > > DT properties compatible with the media video-interface binding and > > things start to play together. > > > > Since this series superseeds the bridge series [1], I have included the > > leftover bindings patch for the ti,ds90c185 here. > > I feel like this series would look better if it would make use of the > proposed bridge media bus format support I have recently sent out [1] > (and which was not there when you first sent v1). > > I understand your fundamental problem here is that the bus format > that should be reported by your bridge is different from the ones > actually supported by the TDA19988 chip, as the wirings ground some > of the input pins. > > Although this is defintely something that could be described in the > bridge's own OF node with the 'bus_width' property, and what I would do, > now that you have made a bridge out from the tda19988 driver, is: > > 1) Set the bridge accepted input bus_format parsing its pin > configuration, or default it if that's not implemented yet. > This will likely be rgb888. You can do that using the trivial > support for bridge input image formats implemented by my series. > 2) Specify in the bridge endpoint a 'bus_width' of <16> > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > and parse the remote endpoint property 'bus_width' and get the <16> > value back. Parsing properties of remote nodes should be avoided as much as possible, as they need to be interpreted in the context of the DT bindings related to the compatible string applicable to that node. I'd rather have the bus_width property in the local endpoint node. > 4) Set the correct format in the atmel hlcd driver to accommodate a > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > or are there other possible combinations I am missing?) > > I would consider this better mostly because in this series you are > creating a semantic for the whole DRM subsystem on the 'bus_width' > property, where numerical values as '12', '16' etc are arbitrary tied > to the selection of a media bus format. At least you should use a > common set of defines [1] between the device tree and the driver, > but this looks anyway fragile imho. This I agree with though. Combining the remote bus format with the local bus width should fix the problem without having to parse remote properties. > Have I maybe missed some parts of the problem you are trying to solve > here? > > Thank you >j > > [1] drm: bridge: Add support for static image formats > https://lwn.net/Articles/752296/ > [2] include/uapi/linux/media-bus-format.h > > > Anyway, this series solves some real issues for my HW. > > > > Cheers, > > Peter > > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > > - patch 2/7 fixed spelling and added an example > > - patch 4/7 parse the DT up front and store the result indexed by encoder > > - old patch 5/6 and 6/6 dropped > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > > - added reviewed-by from Rob to patch 1/6 > > - patch 2/6 changed so that the bus format override is in the endpoint > > DT node, and follows the binding of media video-interfaces. > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > > media video-interface binding (partially). > > - patch 4/6 now makes use of the above helper (and also fixes problems > > with the 3/5 patch from v1 when no override was specified). > > - do not mention unrelated connector display_info details in the cover > > letter and commit messages. > > > > [1] > > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 > > "Bridge" series v1
Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hi Peter, I've been a bit a pain in the arse for you recently, but please bear with me a bit more, and sorry for jumping late on the band wagon. On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > Hi! > > I naively thought that since there was support for both nxp,tda19988 (in > the tda998x driver) and the atmel-hlcdc, things would be a smooth ride. > But it wasn't, so I started looking around and realized I had to fix > things. > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > component, but now in v3 I fix things by making the tda998x driver > a bridge instead. This was after a suggestion from Boris Brezillion > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > after comparing what was needed, I too find the bridge approach better. > > In addition to the above, our PCB interface between the SAMA5D3 and the > HDMI encoder is only using 16 bits, and this has to be described > somewhere, or the atmel-hlcdc driver have no chance of selecting the > correct output mode. Since I have similar problems with a ds90c185 lvds > encoder I added patches to override the atmel-hlcdc output format via > DT properties compatible with the media video-interface binding and > things start to play together. > > Since this series superseeds the bridge series [1], I have included the > leftover bindings patch for the ti,ds90c185 here. > I feel like this series would look better if it would make use of the proposed bridge media bus format support I have recently sent out [1] (and which was not there when you first sent v1). I understand your fundamental problem here is that the bus format that should be reported by your bridge is different from the ones actually supported by the TDA19988 chip, as the wirings ground some of the input pins. Although this is defintely something that could be described in the bridge's own OF node with the 'bus_width' property, and what I would do, now that you have made a bridge out from the tda19988 driver, is: 1) Set the bridge accepted input bus_format parsing its pin configuration, or default it if that's not implemented yet. This will likely be rgb888. You can do that using the trivial support for bridge input image formats implemented by my series. 2) Specify in the bridge endpoint a 'bus_width' of <16> 3) In your atmel-hlcd get both the image format of the bridge (rgb888) and parse the remote endpoint property 'bus_width' and get the <16> value back. 4) Set the correct format in the atmel hlcd driver to accommodate a bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, or are there other possible combinations I am missing?) I would consider this better mostly because in this series you are creating a semantic for the whole DRM subsystem on the 'bus_width' property, where numerical values as '12', '16' etc are arbitrary tied to the selection of a media bus format. At least you should use a common set of defines [1] between the device tree and the driver, but this looks anyway fragile imho. Have I maybe missed some parts of the problem you are trying to solve here? Thank you j [1] drm: bridge: Add support for static image formats https://lwn.net/Articles/752296/ [2] include/uapi/linux/media-bus-format.h > Anyway, this series solves some real issues for my HW. > > Cheers, > Peter > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > - patch 2/7 fixed spelling and added an example > - patch 4/7 parse the DT up front and store the result indexed by encoder > - old patch 5/6 and 6/6 dropped > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > - added reviewed-by from Rob to patch 1/6 > - patch 2/6 changed so that the bus format override is in the endpoint > DT node, and follows the binding of media video-interfaces. > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > media video-interface binding (partially). > - patch 4/6 now makes use of the above helper (and also fixes problems > with the 3/5 patch from v1 when no override was specified). > - do not mention unrelated connector display_info details in the cover > letter and commit messages. > > [1] > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 > "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221 > > Peter Rosin (7): > dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > dt-bindings: display: atmel: optional video-interface of endpoints > drm: of: introduce drm_of_media_bus_fmt > drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes > drm/i2c: tda998x: find the drm_device via the drm_connector > drm/i2c: tda998x: split encoder and component functions from the work > drm/i2c: tda998x: register as a drm bridge > > .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++ > .../bindings/display/bridge/lvds-transmitter.txt | 8 +- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c |
[PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc
Hi! I naively thought that since there was support for both nxp,tda19988 (in the tda998x driver) and the atmel-hlcdc, things would be a smooth ride. But it wasn't, so I started looking around and realized I had to fix things. In v1 and v2 I fixed things by making the atmel-hlcdc driver a master component, but now in v3 I fix things by making the tda998x driver a bridge instead. This was after a suggestion from Boris Brezillion (the atmel-hlcdc maintainer), so there was some risk of bias ... but after comparing what was needed, I too find the bridge approach better. In addition to the above, our PCB interface between the SAMA5D3 and the HDMI encoder is only using 16 bits, and this has to be described somewhere, or the atmel-hlcdc driver have no chance of selecting the correct output mode. Since I have similar problems with a ds90c185 lvds encoder I added patches to override the atmel-hlcdc output format via DT properties compatible with the media video-interface binding and things start to play together. Since this series superseeds the bridge series [1], I have included the leftover bindings patch for the ti,ds90c185 here. Anyway, this series solves some real issues for my HW. Cheers, Peter Changes since v2 https://lkml.org/lkml/2018/4/17/385 - patch 2/7 fixed spelling and added an example - patch 4/7 parse the DT up front and store the result indexed by encoder - old patch 5/6 and 6/6 dropped - patch 5-7/7 are new and makes the tda998x driver a drm_bridge Changes since v1 https://lkml.org/lkml/2018/4/9/294 - added reviewed-by from Rob to patch 1/6 - patch 2/6 changed so that the bus format override is in the endpoint DT node, and follows the binding of media video-interfaces. - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above media video-interface binding (partially). - patch 4/6 now makes use of the above helper (and also fixes problems with the 3/5 patch from v1 when no override was specified). - do not mention unrelated connector display_info details in the cover letter and commit messages. [1] "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221 Peter Rosin (7): dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 dt-bindings: display: atmel: optional video-interface of endpoints drm: of: introduce drm_of_media_bus_fmt drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes drm/i2c: tda998x: find the drm_device via the drm_connector drm/i2c: tda998x: split encoder and component functions from the work drm/i2c: tda998x: register as a drm bridge .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++ .../bindings/display/bridge/lvds-transmitter.txt | 8 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 71 ++-- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 + drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 40 +++- drivers/gpu/drm/drm_of.c | 38 drivers/gpu/drm/i2c/tda998x_drv.c | 201 ++--- include/drm/drm_of.h | 7 + 8 files changed, 342 insertions(+), 51 deletions(-) -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
