[RFR 2/2] drm/panel: Add simple panel support

[Laurent Pinchart]

Hi Thierry,

On Thursday 17 October 2013 14:46:20 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 02:14:45PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:

[snip]

> > The point is that the video pipeline must be described in DT. Having a
> > per-device way to represent connections would be a nightmare to support
> > from an implementation point of view, hence the need for a generic way to
> > describe them.
> 
> Okay, so we're back to the need to describe pipelines in DT. At the risk
> of sounding selfish: I don't care about pipelines. I just want us to
> settle on a way to represent a dumb panel in DT so that it can be
> enabled when it needs to. Furthermore I don't have any hardware that
> exhibits any of these "advanced" features, so I'm totally unqualified to
> work on any of this.
> 
> Can we please try to be a little pragmatic here and solve one problem at
> a time? I am aware that solving this for panels may require some amount
> of foresight, but let's not try to solve everything at once at the risk
> of not getting anything done at all.

I won't force you to care about pipelines, and I don't think this is selfish 
at all :-)

What I would like to ensure is that whatever bindings we come up with, they 
will not preclude us from adding pipeline support when needed (as I'm pretty 
sure it will be needed at some point).

> > > > I'll still keep trying of course, but not for years.I have yet to
> > > > see someone proposing a viable solution to share drivers between DRM
> > > > and V4L2, which is a real pressing requirement, partly due to DT.
> > > 
> > > Yeah... I still don't get that side of the argument. Why exactly do we
> > > need to share the drivers again?
> > 
> > Think about a scaler IP in a SoC or FPGA, with one instance used in a
> > camera capture pipeline, supported by a V4L2 driver, and one instance
> > used in a video output pipeline, supported by a DRM driver. We want a
> > single driver for that IP core. Same story for video encoders.
> 
> Yes, I see. This doesn't immediately concern the simple panel problem that
> I'm trying to solve, so perhaps we can have a separate discussion about it.
> Perhaps this would more easily be solved by providing some sort of helper
> library for the lower level control of the device and wrapping that with
> V4L2 or DRM APIs.

That's more or less the idea of CDF ;-)

> > > If we really need to support display output within V4L2, shouldn't we
> > > instead improve interoperability between the two and use DRM/KMS
> > > exclusively for the output part? Then the problem of having to share
> > > drivers between subsystems goes away and we'll actually be using the
> > > right subsystem for the right job at hand.
> > 
> > In the long term we definitely should improve interoperability between the
> > two. I'd go further than that, having one API for video capture and one
> > API for video output doesn't make much sense, except for historical
> > reasons.
> 
> Having two separate subsystems has worked out pretty well so far. In my
> opinion having strong boundaries between subsystems helps with design.

I'm not saying we've made a mistake. A unified subsystem wouldn't have got it 
right in the first place either anyway. My point is that the boundary has 
collapsed on the hardware side, so I believe the software side will need to 
follow somehow.

> > > Of course none of that is relevant to the topic of DT bindings, which is
> > > what we should probably be concentrating on.
> > 
> > It actually is. Given that DT bindings must describe hardware, the scaler
> > (or encoder, or other entity) would use the same bindings regardless of
> > the Linux subsystem that will be involved. We thus need to make sure the
> > bindings will be usable on both sides.
> 
> Perhaps a single driver could expose both interfaces?

That's one idea that has been proposed. I believe it would be simpler to 
standardize a common API and have translation layers specific to V4L and KMS 
in the core, instead of pushing that translation to all 
panel/bridges/encoders/whatever drivers.

> > > I'm not at all opposed to the idea of CDF or the problems it tries to
> > > address. I don't think it necessarily should be a separate framework for
> > > reasons explained earlier. But even if it were to end up in a separate
> > > framework there's absolutely nothing preventing us from adding a DRM
> > > panel driver that hooks into CDF as a "backend" of sorts for panels that
> > > require something more complex than the simple-panel binding.
> > 
> > Once again it's not about panel having complex needs, but more about using
> > simple panels in complex pipelines. I'm fine with the drm_panel
> > infrastructure, what I would like is DT bindings that describe connections
> > in a more future-proof way. The rest is fine.
> 
> And I've already said elsewhere that the bindings in their current form
> are easily extensible to cater for the needs of CDF.

The simple panel bindings do not include any connection information, so we 
could add that later when needed without having to deprecate, remove or 
repurpose existing properties. The simple panel driver would need to be 
extended, which isn't much of a problem (except that extending it with CDF 
support might require changes to the users of the simple panel driver, which I 
believe won't be happily accepted, but that's a different issue).

My concern is also on the other side. In the patches you've sent the tegra 
driver uses a custom nvidia,panel property to reference the panel. That would 
of course not be CDF-compatible, but there's no way around that at the moment 
if we don't want to keep development of all ARM KMS drivers stalled for the 
next 6 months. It boils down to the question of whether DT should be a stable 
ABI, and I'm increasingly tempted to say that I don't care. I want to solve 
issues we have on the display side, the firmware interface isn't my main 
concern.

> > > But that's precisely the point. Why would we need to go back from the
> > > panel to the display controller? Especially for dumb panels that can't
> > > or don't have to be configured in any way. Even if they needed some sort
> > > of setup, why can't that be done from the display controller/output.
> > > 
> > > Even given a setup where a DSI controller needs to write some registers
> > > in a panel upon initialization, I don't see why the reverse connection
> > > needs to be described. The fact alone that an output dereferences a
> > > panel's phandle should be enough to connect both of them and have any
> > > panel driver use the DSI controller that it's been attached to for the
> > > programming.
> > > 
> > > That's very much analogous to how I2C works. There are no connections
> > > back to the I2C master from the slaves. Yet each I2C client driver
> > > manages to use the services provided by the I2C master to perform
> > > transactions on the I2C bus. In a similar way the DSI controller is the
> > > bus master that talks to DSI panels. DSI panels don't actively talk to
> > > the DSI controller.
> > 
> > It's all about modeling video pipeline graphs in DT. To be able to walk
> > the graph we need to describe connections. Not having bidirectional
> > information means that we restrict the points at which we can start
> > walking the graph, potentially making our life much more difficult in the
> > future.
> > 
> > What's so wrong about adding a port node and link information to the panel
> > DT node ? It describe what's there: the panel has one input, why not make
> > that explicit ?
> 
> What's wrong with it is that there's no way to verify the soundness of
> the design by means of a full implementation because we're missing the
> majority of the pieces. Just putting the nodes into DT to provide some
> imaginary future-proofness isn't helpful either. Without any code that
> actually uses them they are useless.
> 
> And again, why should we add them right away (while not needed) when
> they can easily be added in a backwards-compatible manner at some later
> point when there's actually any use for them and they can actually be
> tested in some larger framework?

It's the "easily" part I'm not sure about. I doubt we'll ever have any easy to 
solve DT backward compatibility issue. However, as mentioned above, this 
shouldn't be a show stopper. I'm thus fine with the way the proposed bindings 
describe (or rather don't describe) the connection. However, I will then 
expect your support in the future to implement the "easy" extension of the 
bindings to support CDF. De we have a deal ? ;-)

> > > > > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > > > > +{
> > > > > > > +	struct panel_simple *p = to_panel_simple(panel);
> > > > > > > +	int err;
> > > > > > > +
> > > > > > > +	if (p->enabled)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	err = regulator_enable(p->supply);
> > > > > > > +	if (err < 0)
> > > > > > > +		dev_err(panel->dev, "failed to enable supply: %d\n", 
err);
> > > > > > 
> > > > > > Is that really a non-fatal error ? Shouldn't the enable operation
> > > > > > return an int ?
> > > > > 
> > > > > There's no way to propagate this in DRM, so why go through the
> > > > > trouble of returning the error? Furthermore, there's nothing that
> > > > > the caller could do to remedy the situation anyway.
> > > > 
> > > > That's a DRM issue, which could be fixed. While the caller can't
> > > > remedy the situation, it should at least report the error to the
> > > > application instead of silently ignoring it.
> > > 
> > > Perhaps. It's not really relevant to the discussion and can always be
> > > fixed in a subsequent patch.
> > 
> > Most things can be fixed by a subsequent patch, that's not a good enough
> > reason not to address the known problems before pushing the code to
> > mainline (to clarify my point of view, there's no need to fix DRM to use
> > the return value before pushing this patch set to mainline, but I'd like
> > a v2 with an int return value).
> 
> Why? What's the use of returning an error if you know up front that it
> can't be used? This should be changed if or when we "fix" DRM to propagate
> errors.

Because not doing so now will require us to change (potentially) lots of panel 
drivers at that time. It's much easier to have each panel driver developer 
implement the required code in his/her driver than having a single developer 
refactoring the code later and have to touch all drivers. If your concern is 
that the error paths won't be testable at the moment, you could easily already 
add a WARN_ON() to the caller to catch problems.

> > > > > > Instead of hardcoding the modes in the driver, which would then
> > > > > > require to be updated for every new simple panel model (and we
> > > > > > know there are lots of them), why don't you specify the modes in
> > > > > > the panel DT node ? The simple panel driver would then become much
> > > > > > more generic. It would also allow board designers to tweak h/v
> > > > > > sync timings depending on the system requirements.
> > > > > 
> > > > > Sigh... we keep second-guessing ourselves. Back at the time when
> > > > > power sequences were designed (and NAKed at the last minute), we all
> > > > > decided that the right thing to do would be to use specific
> > > > > compatible values for individual panels, because that would allow us
> > > > > to encode the power sequencing within the driver. And when we
> > > > > already have the panel model encoded in the compatible value, we
> > > > > might just as well encode the mode within the driver for that panel.
> > > > > Otherwise we'll have to keep adding the same mode timings for every
> > > > > board that uses the same panel.
> > > > > 
> > > > > I do agree though that it might be useful to tweak the mode in case
> > > > > the default one doesn't work. How about we provide a means to
> > > > > override the mode encoded in the driver using one specified in the
> > > > > DT? That's obviously a backwards-compatible change, so it could be
> > > > > added if or when it becomes necessary.
> > > > 
> > > > I share Tomi's point of view here. Your three panels use the same
> > > > power sequence, so I believe we should have a generic panel compatible
> > > > string that would use modes described in DT for the common case. Only
> > > > if a panel required something more complex which can't (or which
> > > > could, but won't for any reason) accurately be described in DT would
> > > > you need to extend the driver.
> > > 
> > > I don't see the point quite frankly. You seem to be assuming that every
> > > panel will only be used in a single board.
> > 
> > No, but in practice that's often the case, at least for boards with .dts
> > files in the mainline kernel.
> > 
> > > However what you're proposing will require the mode timings to be
> > > repeated in every board's DT file, if the same panel is ever used on
> > > more than a single board.
> > 
> > Is that a problem ? You could #include a .dtsi for the panel that would
> > specify the video mode if you want to avoid multiple copies.
> 
> Yes, I don't think it's desirable to duplicate data needlessly in DT. It
> also makes the binding more difficult to use. If I know that the panel
> is one supported by the simple-panel binding, I can just go and add the
> right compatible value without having to bother looking up the mode
> timings and duplicating them. That way DT writers get to concern
> themselves only with the variable data.

I've had a quick chat with Dave Airlie and Hans de Goede yesterday about this. 
As most panels will use standard timings, Hans proposed adding a timings 
property that would reference the standard timings the panel uses (this could 
be a string or integer defined in include/dt-bindings/...). In most case DT 
would just have a single property to select the timings, and only in more 
complex cases where the system designer wants to use custom timings would the 
timings need to be manually defined.

-- 
Regards,

Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131024/8c2fc646/attachment-0001.pgp>