From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Peter Rosin <peda@axentia.se>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Boris Brezillon <boris.brezillon@free-electrons.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
devicetree@vger.kernel.org, David Airlie <airlied@linux.ie>,
Nicolas Ferre <nicolas.ferre@microchip.com>,
dri-devel <dri-devel@lists.freedesktop.org>,
Rob Herring <robh+dt@kernel.org>,
Jacopo Mondi <jacopo@jmondi.org>,
Daniel Vetter <daniel.vetter@intel.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 0/5] allow override of bus format in bridges
Date: Wed, 04 Apr 2018 12:07:15 +0300 [thread overview]
Message-ID: <1563250.XEJxFDuW0h@avalon> (raw)
In-Reply-To: <CAKMK7uH-=BqU+vuePTq4joY3XS2ypvtSKYvpv1LLyovPpSXv6A@mail.gmail.com>
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.
> >> Please also cc Laurent Pinchart on this.
> >>
> >>> Changes since v1 https://lkml.org/lkml/2018/3/17/221
> >>> - Add a proper bridge API to query the bus_format instead of abusing
> >>> the ->get_modes part of the code. This is cleaner but requires
> >>> changes to all display controller drivers wishing to participate.
> >>> - Add patch to adjust the atmel-hlcdc driver according to the above.
> >>> - Hook the new info into the bridge local to the lvds-encoder instead
> >>> of messing about with new interfaces for the panel-bridge driver.
> >>> - Add patch with a DT parsing function of bus_formats in a central
> >>> place.
> >>> - Rephrase the addition of ti,ds90c185 to the lvds-transmitter binding.
> >>>
> >>> Peter Rosin (5):
> >>> dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
> >>> drm: bridge: add API to query the expected input formats of bridges
> >>> drm: of: add display bus-format parser
> >>> drm: bridge: lvds-encoder: allow specifying the input bus format
> >>> drm/atmel-hlcdc: take bridges into account when selecting output
> >>> format
> >>>
> >>> .../bindings/display/bridge/lvds-transmitter.txt | 14 ++++-
> >>> .../devicetree/bindings/display/bus-format.txt | 35 +++++++++++++
> >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 49 +++++++++++++--
> >>> drivers/gpu/drm/bridge/lvds-encoder.c | 25 +++++++++
> >>> drivers/gpu/drm/drm_bridge.c | 32 ++++++++++++
> >>> drivers/gpu/drm/drm_of.c | 59 +++++++++++++++
> >>> include/drm/drm_bridge.h | 18 +++++++
> >>> include/drm/drm_of.h | 9 ++++
> >>> 8 files changed, 237 insertions(+), 4 deletions(-)
> >>> create mode 100644
> >>> Documentation/devicetree/bindings/display/bus-format.txt
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2018-04-04 9:07 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-26 21:24 [PATCH v2 0/5] allow override of bus format in bridges Peter Rosin
2018-03-26 21:24 ` [PATCH v2 1/5] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 Peter Rosin
2018-04-03 22:19 ` Laurent Pinchart
2018-03-26 21:24 ` [PATCH v2 2/5] drm: bridge: add API to query the expected input formats of bridges Peter Rosin
2018-03-27 9:47 ` jacopo mondi
2018-03-27 12:12 ` Peter Rosin
2018-03-27 13:02 ` jacopo mondi
2018-04-04 13:07 ` Laurent Pinchart
2018-04-04 14:40 ` Peter Rosin
2018-03-27 13:04 ` Peter Rosin
2018-03-26 21:24 ` [PATCH v2 3/5] drm: of: add display bus-format parser Peter Rosin
2018-03-26 21:24 ` [PATCH v2 4/5] drm: bridge: lvds-encoder: allow specifying the input bus format Peter Rosin
2018-03-27 10:27 ` jacopo mondi
2018-03-27 12:56 ` Peter Rosin
2018-04-09 18:37 ` Rob Herring
2018-03-26 21:24 ` [PATCH v2 5/5] drm/atmel-hlcdc: take bridges into account when selecting output format Peter Rosin
2018-03-28 7:08 ` [PATCH v2 0/5] allow override of bus format in bridges Daniel Vetter
2018-04-03 22:28 ` Laurent Pinchart
2018-04-03 22:31 ` Laurent Pinchart
2018-04-04 6:34 ` Daniel Vetter
2018-04-04 9:07 ` Laurent Pinchart [this message]
2018-04-04 12:35 ` Peter Rosin
2018-04-09 7:04 ` Peter Rosin
2018-04-09 12:51 ` Laurent Pinchart
2018-04-09 13:29 ` Peter Rosin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1563250.XEJxFDuW0h@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=airlied@linux.ie \
--cc=alexandre.belloni@bootlin.com \
--cc=boris.brezillon@free-electrons.com \
--cc=daniel.vetter@intel.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jacopo@jmondi.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nicolas.ferre@microchip.com \
--cc=peda@axentia.se \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).