Re: [PATCH v4 16/16] drm/panel: Add Sharp LQ101R1SX01 support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7834 bytes --]

On Tue, Nov 04, 2014 at 11:41:15AM +0100, Andrzej Hajda wrote:
> On 11/03/2014 10:13 AM, Thierry Reding wrote:
[...]
> > diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
[...]
> > +Example:
> > +
> > +	dsi@54300000 {
> > +		panel: panel@0 {
> > +			compatible = "sharp,lq101r1sx01";
> > +			reg = <0>;
> > +
> > +			link2 = <&secondary>;
> > +
> > +			power-supply = <...>;
> > +			backlight = <...>;
> > +		};
> > +	};
> > +
> > +	dsi@54400000 {
> > +		secondary: panel@0 {
> > +			compatible = "sharp,lq101r1sx01";
> > +			reg = <0>;
> > +		};
> > +	};
> 
> The bindings above and the implementation below clearly shows
> that the secondary node is just a dummy dsi device.

It's not only a dummy device. Binding it to the device's compatible
string allows the driver to properly set up the DSI device (flags,
number of lanes, ...).

> Hiding this behind conditionals in both docs and code does not look good
> to me. On the other side having common dummy dsi driver
> would allow to reuse it in other dual-dsi devices.
> So instead of 2nd node you would have:
> 	dsi@54400000 {
> 		secondary: panel@0 {
> 			compatible = "dsi-dummy-whatever";
> 			reg = <0>;
> 		};
> 	};
> 
> Or just:
> 	dsi2: dsi@54400000 {
> 	}
> 
> with phandle to dsi2 in 1st node, in such case 2nd dsi dev would be
> created dynamically like with i2c_new_dummy.

I don't think that's technically valid DT. Every device must have a
compatible property. And the compatible property should have an entry
for the specific model of the device. "dummy" doesn't qualify for that

Hopefully when more dual-channel DSI devices get supported some kind of
pattern will emerge. For now I'll go with what I have in this patch. It
is as accurate as I can think of.

> > diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
[...]
> > +struct sharp_panel {
> > +	struct drm_panel base;
> > +	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
> > +	struct mipi_dsi_device *link1;
> > +	struct mipi_dsi_device *link2;
> > +
> > +	struct backlight_device *backlight;
> > +	struct regulator *supply;
> > +
> > +	bool prepared;
> > +	bool enabled;
> 
> Nitpick.
> As I wrote in review to previous version it is up to panel client
> (usually connector) to call panel callbacks properly, we should not add
> additional checks to panels. If call order is incorrect then the client
> should be fixed.

There are no such guarantees today. But hopefully this will change with
atomic modesetting.

> > +static int sharp_panel_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> > +	int err;
> > +
> > +	/* only detach from host for the DSI-LINK2 interface */
> > +	if (!sharp) {
> > +		mipi_dsi_detach(dsi);
> 
> Quotation from previous review:
> 
> >> There is no locking mechanism here, so it is possible that
> >> everything can crash if someone unbind driver from LINK2 and then try to
> >> enable the panel.
> > 
> > I don't think so. Since we're not doing anything with the DSI-LINK2
> > device anymore it's irrelevant whether it is bound to the driver or
> > not.
> > 
> 
> Please consider following scenario:
> 1. panel link2 is probed
> 2. panel link1 is probed
> 3. panel link2 is unbound (for example by: echo link2
> >/sys/bus/dsi/drivers/*lq101*/unbind)
> 4. drm is bound:
>    - during sharp_setup_symmetrical_split you will call transfer
>      but there is no device attached to dsi2.
> 
> Maybe it will not cause any troubles but it seems incorrect.

The device is still there. The driver may not be bound to it, but we
don't need that for the transfers anyway.

> I guess simple workaround is to do device_lock and check if device is
> bound every time you want to access link2 device.

I don't see where the problem is. We do keep a reference to the LINK2
device from the first, so it can't just disappear. The only way it could
disappear is if the host controller that provides its bus goes away, but
there's code at the DSI host controller level to deal with that.

> > +		return 0;
> > +	}
> > +
> > +	err = sharp_panel_disable(&sharp->base);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
> > +
> > +	err = mipi_dsi_detach(dsi);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
> > +
> > +	drm_panel_detach(&sharp->base);
> > +	sharp_panel_del(sharp);
> 
> You have following flow:
> 
> sharp_probe:
> 	drm_panel_add
> 	dsi_attach
> 
> sharp_remove:
> 	drm_panel_disable
> 	dsi_detach
> 	drm_panel_detach
> 	drm_panel_del
> 
> So panel initialization is done by connector
> but panel de-initialization is done by the panel itself,

No. The panel registers with the panel registry so that the connector
can find it. But when the panel registers it doesn't know anything about
the DRM device it is going to be attached to, so we have to attach it to
the connector only when that becomes available.

When the driver is unloaded, the panel unregisters from the registry.
Also it needs to detach from a connector if it's still connected so that
the connector can be marked unconnected. Similarly when the connector
goes away it must let the panel know that it's being detached.

But it looks like this infrastructure is generally somewhat immature.
For example this only works relatively well for DSI panels because we
have the DSI peripheral attach/detach callbacks. For RGB or LVDS panels
we have no such thing, so there's no way currently for these panels to
hotplug.

> on the other side connector can also disable/detach the panel.
> So it seems that drm_panel resource ownership is split between
> the panel and the connector. It seems racy, unless you provide
> additional synchronization mechanisms.
> And indeed there are races here, for example there are two processes:
> - (1) connector calls sharp_prepare, in the middle of
> sharp_setup_symmetrical_split process goes to sleep,
> - (2) sharp_panel_remove is called due to unbind, or module removal,
> sharp_disable is called,
> - (1) process wakes up, it tries to continue
> sharp_setup_symmetrical_split, results are unpredictable.

That's nothing you can solve by simply changing the calling order here.
DRM panel is completely missing any infrastructure to allow you to
safely deal with that kind of situation.

But it's something that I've been trying to fix for the past couple of
days.

> If you leave ownership of drm_panel to connector you can reuse
> synchronization mechanisms of connector/drm inside connector. Otherwise
> you will need to add another locks in drm_panel.
> 
> You can look at exynos_drm_dsi.c code, especially:
> - exynos_dsi_detect,
> - exynos_dsi_host_attach,
> - exynos_dsi_host_detach,
> 
> and usage of dsi->panel_node and dsi->panel with drm synchronization via
> drm_helper_hpd_irq_event.

There are a number of ways that one could possibly implement this. I do
not think any of the existing ones is as good as it could be and I think
we really ought to standardize on how to do this to make it easier for
other drivers to adapt DRM panels (and bridges for that matter).

Unfortunately before we have a good plan on how to handle panels more
uniformly I don't see how we can possibly make everybody happy. It seems
like currently whatever panel is written for Exynos isn't going to work
on Tegra and vice-versa. So how about we merge this series and I'll look
into how we can unify things?

I'd appreciate any ideas on how such a unification could look.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel