On 2018-04-04 11:07, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Wednesday, 4 April 2018 09:34:41 EEST Daniel Vetter wrote:
>> On Wed, Apr 4, 2018 at 12:28 AM, Laurent Pinchart wrote:
>>> On Wednesday, 28 March 2018 10:08:26 EEST Daniel Vetter wrote:
>>>> On Mon, Mar 26, 2018 at 11:24:42PM +0200, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> [I got to v2 sooner than expected]
>>>>>
>>>>> I have an Atmel sama5d31 hooked up to an lvds encoder and then
>>>>> on to an lvds panel. Which seems like something that has been
>>>>> done one or two times before...
>>>>>
>>>>> The problem is that the bus_format of the SoC and the panel do
>>>>> not agree. The SoC driver (atmel-hlcdc) can handle the
>>>>> rgb444, rgb565, rgb666 and rgb888 bus formats. The hardware is
>>>>> wired for the rgb565 case. The lvds encoder supports rgb888 on
>>>>> its input side with the LSB wires for each color simply pulled
>>>>> down internally in the encoder in my case which means that the
>>>>> rgb565 bus_format is the format that works best. And the panel
>>>>> is expecting lvds (vesa-24), which is what the encoder outputs.
>>>>>
>>>>> The reason I "blame" the bus_format of the drm_connector is that
>>>>> with the below DT snippet, things do not work *exactly* due to
>>>>> that. At least, it starts to work if I hack the panel-lvds driver
>>>>> to report the rgb565 bus_format instead of vesa-24.
>>>>>
>>>>> panel: panel {
>>>>> compatible = "panel-lvds";
>>>>>
>>>>> width-mm = <304>;
>>>>> height-mm = <228;
>>>>>
>>>>> data-mapping = "vesa-24";
>>>>>
>>>>> panel-timing {
>>>>> // 1024x768 @ 60Hz (typical)
>>>>> clock-frequency = <52140000 65000000 71100000>;
>>>>> hactive = <1024>;
>>>>> vactive = <768>;
>>>>> hfront-porch = <48 88 88>;
>>>>> hback-porch = <96 168 168>;
>>>>> hsync-len = <32 64 64>;
>>>>> vfront-porch = <8 13 14>;
>>>>> vback-porch = <8 13 14>;
>>>>> vsync-len = <8 12 14>;
>>>>> };
>>>>>
>>>>> port {
>>>>> panel_input: endpoint {
>>>>> remote-endpoint = <&lvds_encoder_output>;
>>>>> };
>>>>> };
>>>>> };
>>>>>
>>>>> lvds-encoder {
>>>>> compatible = "ti,ds90c185", "lvds-encoder";
>>>>>
>>>>> ports {
>>>>> #address-cells = <1>;
>>>>> #size-cells = <0>;
>>>>>
>>>>> port@0 {
>>>>> reg = <0>;
>>>>>
>>>>> lvds_encoder_input: endpoint {
>>>>> remote-endpoint = <&hlcdc_output>;
>>>>> };
>>>>> };
>>>>>
>>>>> port@1 {
>>>>> reg = <1>;
>>>>>
>>>>> lvds_encoder_output: endpoint {
>>>>> remote-endpoint = <&panel_input>;
>>>>> };
>>>>> };
>>>>> };
>>>>> };
>>>>>
>>>>> But, instead of perverting the panel-lvds driver with support
>>>>> for a totally fake non-lvds bus_format, I intruduce an API that allows
>>>>> display controller drivers to query the required bus_format of any
>>>>> intermediate bridges, and match up with that instead of the formats
>>>>> given by the drm_connector. I trigger this with this addition to the
>>>>>
>>>>> lvds-encoder DT node:
>>>>> interface-pix-fmt = "rgb565";
>>>>>
>>>>> Naming is hard though, so I'm not sure if that's good?
>>>>>
>>>>> I threw in the first patch, since that is the actual lvds encoder
>>>>> I have in this case.
>>>>>
>>>>> Suggestions welcome.
>>>>
>>>> Took a quick look, feels rather un-atomic. And there's beend discussing
>>>> for other bridge related state that we might want to track (like the full
>>>> adjusted_mode that might need to be adjusted at each stage in the chain).
>>>> So here's my suggestions:
>>>>
>>>> - Add an optional per-bridge internal state struct using the support in
>>>> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-> >> private-state
>>>>
>>>> Yes it says "driver private", but since bridge is just helper stuff
>>>> that's all included. "driver private" == "not exposed as uapi" here.
>>>> Include all the usual convenience wrappers to get at the state for a
>>>> bridge.
>>>>
>>>> - Then stuff your bus_format into that new drm_bridge_state struct.
>>>>
>>>> - Add a new bridge callback atomic_check, which gets that bridge state as
>>>> parameter (similar to all the other atomic_check functions).
>>>>
>>>> This way we can even handle the bus_format dynamically, through the
>>>> atomic framework your bridge's atomic_check callback can look at the
>>>> entire atomic state (both up and down the chain if needed), it all neatly
>>>> fits into atomic overall and it's much easier to extend.
>>>
>>> While I think we'll eventually need bridge states, I don't think that's
>>> need yet. The bus formats reported by this patch series are static. We're
>>> not talking about the currently configured format for a bridge, but about
>>> the list of supported formats. This is similar to the bus_formats field
>>> present in the drm_display_info structure. There is thus in my opinion no
>>> need to interface this with atomic until we need to track the current
>>> format (and I think that will indeed happen at some point, but I don't
>>> think Peter needs this feature for now). That's why I've told Peter that
>>> I would like a bridge API to report the information and haven't requested
>>> a state-based implementation.
>>
>> If it's static, why a dynamic callback? Just add it to struct
>> drm_bridge, require it's filled out before registering the bridge,
>> done.
>
> If I remember correctly I mentioned both options in my initial proposal,
> without a personal preference. A new field in struct drm_bridge would
> certainly work for me.
You did. Ok, so v3 coming up...
Cheers,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
