Message-ID: <20180420112204.GK4235@w540>
Date:   Fri, 20 Apr 2018 13:22:04 +0200
From:   jacopo mondi <jacopo@...ndi.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Peter Rosin <peda@...ntia.se>, linux-kernel@...r.kernel.org,
        David Airlie <airlied@...ux.ie>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Sean Paul <seanpaul@...omium.org>,
        Russell King <linux@...linux.org.uk>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: 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
> > >   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(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists