Re: [PATCH v2 45/49] drm/omap: Add support for drm_bridge

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

On 08/02/2019 17:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, Jan 18, 2019 at 12:33:03PM +0200, Tomi Valkeinen wrote:
>> On 11/01/19 05:51, Laurent Pinchart wrote:
>>> Hook up drm_bridge support in the omapdrm driver. Despite the recent
>>> extensive preparation work, this is a rather intrusive change, as the
>>> management of outputs needs to be adapted through the driver to handle
>>> both omap_dss_device and drm_bridge.
>>>
>>> Connector creation is skipped when using a drm_bridge, as the bridge
>>> creates the connector internally. This creates issues with systems that
>>> split connector operations (such as modes retrieval and hot-plug
>>> detection) across different bridges. These systems can't be supported
>>> using drm_bridge for now (their support through the omap_dss_device
>>> infrastructure is not affected), this will be fixed in subsequent
>>> changes.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>>
>> <snip>
>>
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
>>> index f25ecfd26534..2a53025c2fde 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/output.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
>>> @@ -20,25 +20,34 @@
>>>  #include <linux/platform_device.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/of.h>
>>> +#include <linux/of_graph.h>
>>>  
>>>  #include "dss.h"
>>>  #include "omapdss.h"
>>>  
>>>  int omapdss_device_init_output(struct omap_dss_device *out)
>>>  {
>>> -	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
>>> -	if (IS_ERR(out->next)) {
>>> -		if (PTR_ERR(out->next) != -EPROBE_DEFER)
>>> -			dev_err(out->dev, "failed to find video sink\n");
>>> -		return PTR_ERR(out->next);
>>> +	struct device_node *remote_node;
>>> +
>>> +	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
>>> +	if (!remote_node) {
>>> +		dev_dbg(out->dev, "failed to find video sink\n");
>>> +		return 0;
>>>  	}
>>
>> This breaks boards that have displays/bridges connected to ports above
>> 0. For example, DPI output has 3 ports on some SoCs.
> 
> Does it break them, or are they already broken ?

Good point. It's very well possible it was already broken. It worked in
4.14, that I know =). That didn't have
omapdss_of_find_connected_device() yet. So possibly it's
27d624527d99265c2df999af3615ff71c29d06f4 that breaks it.

I think we don't have many boards that use non-0 port for DPI. The one I
have, DRA71 EVM, doesn't have mainline support, and I have only ran it
on 4.14 before this.

And OMAP3 SDI uses port 1, but it handles this one specifically.

>> I made a quick fix like so:
>>
>> +       port_num = __ffs(out->of_ports);
>> +
>> +       remote_node = of_graph_get_remote_node(out->dev->of_node,
>> port_num, 0);
>>
>> Which I think works for all our outputs as we only set a single bit in
>> of_ports for outputs. But I don't think that's quite correct. Should we
>> have another field which tells which port is to be used? Then again,
>> maybe __ffs() is good enough here, as we can guarantee that there's ever
>> a single port in of_ports.
> 
> Down the road I think it would make sense to replace of_ports with
> of_port. I thought we can't do so right now as we have three encoder
> drivers that set two bits in of_ports, but now that I've double-checked,
> of_ports isn't used. We could thus replace it, and only set it for the
> internal DSS devices.

Yes, this would clean up the code a bit.

> If my analysis is correct and the problem isn't introduced by this
> patch, could it be fixed on top of the series ?

Yes, that sounds fine.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel