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

[Andrzej Hajda]

On 11/04/2014 02:29 PM, Thierry Reding wrote:
> 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 at 54300000 {
>>> +		panel: panel at 0 {
>>> +			compatible = "sharp,lq101r1sx01";
>>> +			reg = <0>;
>>> +
>>> +			link2 = <&secondary>;
>>> +
>>> +			power-supply = <...>;
>>> +			backlight = <...>;
>>> +		};
>>> +	};
>>> +
>>> +	dsi at 54400000 {
>>> +		secondary: panel at 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, ...).

This is why I called it dummy DSI device :)

> 
>> 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 at 54400000 {
>> 		secondary: panel at 0 {
>> 			compatible = "dsi-dummy-whatever";
>> 			reg = <0>;
>> 		};
>> 	};
>>
>> Or just:
>> 	dsi2: dsi at 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

One could ask if it is technically valid to represent one hw device via
two separate nodes. Each choice scarifies something.

> 
> 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.

Agreed


> 
>>> 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.

I do not understand that. It is the connector code who calls these
callbacks. Why cannot you force it to call them in proper order?

> 
>>> +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.

And you do not want to call it dummy device :)

> 
>> 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.
> 

As I understand it is important to you that host is set-up according to
settings passed during LINK2 device attachment.
IMHO assumption that the host will keep these settings after device is
detached is incorrect. The host can return to its initial state or any
other state. I think that it is valid if the host refuses transfers on
'non-attached' channels.


>>> +		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.

This is why I have proposed interface_tracker framework, we would have
hotplug/hotunplug callbacks already :)

> 
>> 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.

This is not true for DSI panels - attach/detach callbacks allows to do
it correctly.

> 
> 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 so. There are simple rules of synchronization, if you
follow them you will usually end up with similar design.


> 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?

This is not about compatibility between different hosts and panels, this
is just about making sharp panel driver work correctly with tegra dsi host.
I think it is quite easy to fix and it would be good to do it before
merging.

Unification is another issue which can be solved later.

Regards
Andrzej

> 
> I'd appreciate any ideas on how such a unification could look.
> 
> Thierry
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>