[PATCH 0/7] RPi touchscreen as a panel driver again

[PATCH 0/7] RPi touchscreen as a panel driver again

[Eric Anholt]

After splitting the panel driver out into a panel and bridge due to
panel review, the feedback from bridge maintainers was that it didn't
make sense as a bridge.  I completely agree with them.  This series
returns the driver to being a panel, but this time probing as an i2c
client rather than a DSI device (due to review from bridge
submission).

Eric Anholt (7):
  drm/bridge: Support hotplugging panel-bridge.
  drm/vc4: Fix DSI T_INIT timing.
  drm/vc4: Fix misleading name of the continuous flag.
  drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.
  dt-bindings: Document the Raspberry Pi Touchscreen nodes.
  drm/panel: Add support for the Raspberry Pi 7" Touchscreen.
  ARM: dts: bcm2835: Enable the Raspberry Pi touchscreen panel.

 .../panel/raspberrypi,7inch-touchscreen.txt        |  49 ++
 arch/arm/boot/dts/bcm2835-rpi-b-plus.dts           |   5 +
 arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts           |   5 +
 arch/arm/boot/dts/bcm2835-rpi-b.dts                |   5 +
 arch/arm/boot/dts/bcm2836-rpi-2-b.dts              |   5 +
 arch/arm/boot/dts/bcm283x.dtsi                     |  30 +-
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts   |   5 +
 drivers/gpu/drm/bridge/panel.c                     |   3 +
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 507 +++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_dsi.c                      |  24 +-
 12 files changed, 640 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

-- 
2.11.0

[PATCH 0/7] RPi touchscreen as a panel driver again

[Eric Anholt]

After splitting the panel driver out into a panel and bridge due to
panel review, the feedback from bridge maintainers was that it didn't
make sense as a bridge.  I completely agree with them.  This series
returns the driver to being a panel, but this time probing as an i2c
client rather than a DSI device (due to review from bridge
submission).

Eric Anholt (7):
  drm/bridge: Support hotplugging panel-bridge.
  drm/vc4: Fix DSI T_INIT timing.
  drm/vc4: Fix misleading name of the continuous flag.
  drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.
  dt-bindings: Document the Raspberry Pi Touchscreen nodes.
  drm/panel: Add support for the Raspberry Pi 7" Touchscreen.
  ARM: dts: bcm2835: Enable the Raspberry Pi touchscreen panel.

 .../panel/raspberrypi,7inch-touchscreen.txt        |  49 ++
 arch/arm/boot/dts/bcm2835-rpi-b-plus.dts           |   5 +
 arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts           |   5 +
 arch/arm/boot/dts/bcm2835-rpi-b.dts                |   5 +
 arch/arm/boot/dts/bcm2836-rpi-2-b.dts              |   5 +
 arch/arm/boot/dts/bcm283x.dtsi                     |  30 +-
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts   |   5 +
 drivers/gpu/drm/bridge/panel.c                     |   3 +
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 507 +++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_dsi.c                      |  24 +-
 12 files changed, 640 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

-- 
2.11.0

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

[PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

If the panel-bridge is being set up after the drm_mode_config_reset(),
then the connector's state would never get initialized, and we'd
dereference the NULL in the hotplug path.  We also need to register
the connector, so that userspace can get at it.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/bridge/panel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 67fe19e5a9c6..8ed8a70799c7 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge)
 
 	drm_mode_connector_attach_encoder(&panel_bridge->connector,
 					  bridge->encoder);
+	drm_atomic_helper_connector_reset(&panel_bridge->connector);
 
 	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
 	if (ret < 0)
 		return ret;
 
+	drm_connector_register(&panel_bridge->connector);
+
 	return 0;
 }
 
-- 
2.11.0

[PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

If the panel-bridge is being set up after the drm_mode_config_reset(),
then the connector's state would never get initialized, and we'd
dereference the NULL in the hotplug path.  We also need to register
the connector, so that userspace can get at it.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/bridge/panel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 67fe19e5a9c6..8ed8a70799c7 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge)
 
 	drm_mode_connector_attach_encoder(&panel_bridge->connector,
 					  bridge->encoder);
+	drm_atomic_helper_connector_reset(&panel_bridge->connector);
 
 	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
 	if (ret < 0)
 		return ret;
 
+	drm_connector_register(&panel_bridge->connector);
+
 	return 0;
 }
 
-- 
2.11.0

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/16/2017 02:11 AM, Eric Anholt wrote:
> If the panel-bridge is being set up after the drm_mode_config_reset(),
> then the connector's state would never get initialized, and we'd
> dereference the NULL in the hotplug path.  We also need to register
> the connector, so that userspace can get at it.
> 

Shouldn't the KMS driver make sure the panel-bridge is set up before
drm_mode_config_reset? Is it the case when we're inserting the
panel-bridge driver as a module?


All the connectors that have been added are registered automatically
when drm_dev_register() is called by the KMS driver. Registering a
connector in the middle of setting up our driver is prone to race
conditions if the userspace decides to use them immediately.

Thanks,
Archit

> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>   drivers/gpu/drm/bridge/panel.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 67fe19e5a9c6..8ed8a70799c7 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge)
>   
>   	drm_mode_connector_attach_encoder(&panel_bridge->connector,
>   					  bridge->encoder);
> +	drm_atomic_helper_connector_reset(&panel_bridge->connector);
>   
>   	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
>   	if (ret < 0)
>   		return ret;
>   
> +	drm_connector_register(&panel_bridge->connector);
> +
>   	return 0;
>   }
>   
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/16/2017 02:11 AM, Eric Anholt wrote:
> If the panel-bridge is being set up after the drm_mode_config_reset(),
> then the connector's state would never get initialized, and we'd
> dereference the NULL in the hotplug path.  We also need to register
> the connector, so that userspace can get at it.
> 

Shouldn't the KMS driver make sure the panel-bridge is set up before
drm_mode_config_reset? Is it the case when we're inserting the
panel-bridge driver as a module?


All the connectors that have been added are registered automatically
when drm_dev_register() is called by the KMS driver. Registering a
connector in the middle of setting up our driver is prone to race
conditions if the userspace decides to use them immediately.

Thanks,
Archit

> Signed-off-by: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> ---
>   drivers/gpu/drm/bridge/panel.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 67fe19e5a9c6..8ed8a70799c7 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge)
>   
>   	drm_mode_connector_attach_encoder(&panel_bridge->connector,
>   					  bridge->encoder);
> +	drm_atomic_helper_connector_reset(&panel_bridge->connector);
>   
>   	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
>   	if (ret < 0)
>   		return ret;
>   
> +	drm_connector_register(&panel_bridge->connector);
> +
>   	return 0;
>   }
>   
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Archit Taneja <architt@codeaurora.org> writes:

> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>> then the connector's state would never get initialized, and we'd
>> dereference the NULL in the hotplug path.  We also need to register
>> the connector, so that userspace can get at it.
>> 
>
> Shouldn't the KMS driver make sure the panel-bridge is set up before
> drm_mode_config_reset? Is it the case when we're inserting the
> panel-bridge driver as a module?
>
>
> All the connectors that have been added are registered automatically
> when drm_dev_register() is called by the KMS driver. Registering a
> connector in the middle of setting up our driver is prone to race
> conditions if the userspace decides to use them immediately.

Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
which in the case of a panel module that creates the DSI device
(adv7533-style, like you said I should use as a reference) will be after
drm_mode_config_reset() and drm_dev_register().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

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

Archit Taneja <architt@codeaurora.org> writes:

> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>> then the connector's state would never get initialized, and we'd
>> dereference the NULL in the hotplug path.  We also need to register
>> the connector, so that userspace can get at it.
>> 
>
> Shouldn't the KMS driver make sure the panel-bridge is set up before
> drm_mode_config_reset? Is it the case when we're inserting the
> panel-bridge driver as a module?
>
>
> All the connectors that have been added are registered automatically
> when drm_dev_register() is called by the KMS driver. Registering a
> connector in the middle of setting up our driver is prone to race
> conditions if the userspace decides to use them immediately.

Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
which in the case of a panel module that creates the DSI device
(adv7533-style, like you said I should use as a reference) will be after
drm_mode_config_reset() and drm_dev_register().

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/16/2017 08:13 PM, Eric Anholt wrote:
> Archit Taneja <architt@codeaurora.org> writes:
> 
>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>> then the connector's state would never get initialized, and we'd
>>> dereference the NULL in the hotplug path.  We also need to register
>>> the connector, so that userspace can get at it.
>>>
>>
>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>> drm_mode_config_reset? Is it the case when we're inserting the
>> panel-bridge driver as a module?
>>
>>
>> All the connectors that have been added are registered automatically
>> when drm_dev_register() is called by the KMS driver. Registering a
>> connector in the middle of setting up our driver is prone to race
>> conditions if the userspace decides to use them immediately.
> 
> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> which in the case of a panel module that creates the DSI device
> (adv7533-style, like you said I should use as a reference) will be after
> drm_mode_config_reset() and drm_dev_register().

Okay. In the case of the msm kms driver, we defer probe until the
adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
and drm_dev_register(). I assumed this was the general practice followed by
most kms drivers. I.,e the kms driver defers probe until all connector
related modules are inserted, and only then proceed to create a drm device.

Feedback from others on this would be appreciated, though.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/16/2017 08:13 PM, Eric Anholt wrote:
> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
> 
>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>> then the connector's state would never get initialized, and we'd
>>> dereference the NULL in the hotplug path.  We also need to register
>>> the connector, so that userspace can get at it.
>>>
>>
>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>> drm_mode_config_reset? Is it the case when we're inserting the
>> panel-bridge driver as a module?
>>
>>
>> All the connectors that have been added are registered automatically
>> when drm_dev_register() is called by the KMS driver. Registering a
>> connector in the middle of setting up our driver is prone to race
>> conditions if the userspace decides to use them immediately.
> 
> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> which in the case of a panel module that creates the DSI device
> (adv7533-style, like you said I should use as a reference) will be after
> drm_mode_config_reset() and drm_dev_register().

Okay. In the case of the msm kms driver, we defer probe until the
adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
and drm_dev_register(). I assumed this was the general practice followed by
most kms drivers. I.,e the kms driver defers probe until all connector
related modules are inserted, and only then proceed to create a drm device.

Feedback from others on this would be appreciated, though.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Laurent Pinchart]

Hi Archit,

On Tuesday 20 Jun 2017 09:18:00 Archit Taneja wrote:
> On 06/16/2017 08:13 PM, Eric Anholt wrote:
> > Archit Taneja <architt@codeaurora.org> writes:
> >> On 06/16/2017 02:11 AM, Eric Anholt wrote:
> >>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>> then the connector's state would never get initialized, and we'd
> >>> dereference the NULL in the hotplug path.  We also need to register
> >>> the connector, so that userspace can get at it.
> >> 
> >> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >> drm_mode_config_reset? Is it the case when we're inserting the
> >> panel-bridge driver as a module?
> >> 
> >> 
> >> All the connectors that have been added are registered automatically
> >> when drm_dev_register() is called by the KMS driver. Registering a
> >> connector in the middle of setting up our driver is prone to race
> >> conditions if the userspace decides to use them immediately.
> > 
> > Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> > which in the case of a panel module that creates the DSI device
> > (adv7533-style, like you said I should use as a reference) will be after
> > drm_mode_config_reset() and drm_dev_register().
> 
> Okay. In the case of the msm kms driver, we defer probe until the
> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> and drm_dev_register(). I assumed this was the general practice followed by
> most kms drivers. I.,e the kms driver defers probe until all connector
> related modules are inserted, and only then proceed to create a drm device.

I'd love to see support for a more dynamic approach that would allow 
registering outputs at runtime. Until that's implemented, however, I agree 
with your statement, drivers should wait until all components are available 
before registering the DRM device.

> Feedback from others on this would be appreciated, though.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Laurent Pinchart]

Hi Archit,

On Tuesday 20 Jun 2017 09:18:00 Archit Taneja wrote:
> On 06/16/2017 08:13 PM, Eric Anholt wrote:
> > Archit Taneja <architt@codeaurora.org> writes:
> >> On 06/16/2017 02:11 AM, Eric Anholt wrote:
> >>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>> then the connector's state would never get initialized, and we'd
> >>> dereference the NULL in the hotplug path.  We also need to register
> >>> the connector, so that userspace can get at it.
> >> 
> >> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >> drm_mode_config_reset? Is it the case when we're inserting the
> >> panel-bridge driver as a module?
> >> 
> >> 
> >> All the connectors that have been added are registered automatically
> >> when drm_dev_register() is called by the KMS driver. Registering a
> >> connector in the middle of setting up our driver is prone to race
> >> conditions if the userspace decides to use them immediately.
> > 
> > Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> > which in the case of a panel module that creates the DSI device
> > (adv7533-style, like you said I should use as a reference) will be after
> > drm_mode_config_reset() and drm_dev_register().
> 
> Okay. In the case of the msm kms driver, we defer probe until the
> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> and drm_dev_register(). I assumed this was the general practice followed by
> most kms drivers. I.,e the kms driver defers probe until all connector
> related modules are inserted, and only then proceed to create a drm device.

I'd love to see support for a more dynamic approach that would allow 
registering outputs at runtime. Until that's implemented, however, I agree 
with your statement, drivers should wait until all components are available 
before registering the DRM device.

> Feedback from others on this would be appreciated, though.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

Archit Taneja <architt@codeaurora.org> writes:

> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>> Archit Taneja <architt@codeaurora.org> writes:
>> 
>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>> then the connector's state would never get initialized, and we'd
>>>> dereference the NULL in the hotplug path.  We also need to register
>>>> the connector, so that userspace can get at it.
>>>>
>>>
>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>> drm_mode_config_reset? Is it the case when we're inserting the
>>> panel-bridge driver as a module?
>>>
>>>
>>> All the connectors that have been added are registered automatically
>>> when drm_dev_register() is called by the KMS driver. Registering a
>>> connector in the middle of setting up our driver is prone to race
>>> conditions if the userspace decides to use them immediately.
>> 
>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>> which in the case of a panel module that creates the DSI device
>> (adv7533-style, like you said I should use as a reference) will be after
>> drm_mode_config_reset() and drm_dev_register().
>
> Okay. In the case of the msm kms driver, we defer probe until the
> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> and drm_dev_register(). I assumed this was the general practice followed by
> most kms drivers. I.,e the kms driver defers probe until all connector
> related modules are inserted, and only then proceed to create a drm device.

The problem, though, is the panel driver needs the MIPI DSI host to
exist to call mipi_dsi_device_register_full() during the probe process.
The adv7533 driver gets around this by registering the DSI device in the
bridge attach step, but drm_panel doesn't have an attach step.

Another alternative is my original version of the panel driver that was
a mipi_dsi_device driver that registered the panel during the DSI device
probe.  That's why vc4's panel lookup is during the MIPI DSI attach
phase, currently.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:

> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>> 
>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>> then the connector's state would never get initialized, and we'd
>>>> dereference the NULL in the hotplug path.  We also need to register
>>>> the connector, so that userspace can get at it.
>>>>
>>>
>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>> drm_mode_config_reset? Is it the case when we're inserting the
>>> panel-bridge driver as a module?
>>>
>>>
>>> All the connectors that have been added are registered automatically
>>> when drm_dev_register() is called by the KMS driver. Registering a
>>> connector in the middle of setting up our driver is prone to race
>>> conditions if the userspace decides to use them immediately.
>> 
>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>> which in the case of a panel module that creates the DSI device
>> (adv7533-style, like you said I should use as a reference) will be after
>> drm_mode_config_reset() and drm_dev_register().
>
> Okay. In the case of the msm kms driver, we defer probe until the
> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> and drm_dev_register(). I assumed this was the general practice followed by
> most kms drivers. I.,e the kms driver defers probe until all connector
> related modules are inserted, and only then proceed to create a drm device.

The problem, though, is the panel driver needs the MIPI DSI host to
exist to call mipi_dsi_device_register_full() during the probe process.
The adv7533 driver gets around this by registering the DSI device in the
bridge attach step, but drm_panel doesn't have an attach step.

Another alternative is my original version of the panel driver that was
a mipi_dsi_device driver that registered the panel during the DSI device
probe.  That's why vc4's panel lookup is during the MIPI DSI attach
phase, currently.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Benjamin Gaignard]

2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
> Archit Taneja <architt@codeaurora.org> writes:
>
>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>> Archit Taneja <architt@codeaurora.org> writes:
>>>
>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>> then the connector's state would never get initialized, and we'd
>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>> the connector, so that userspace can get at it.
>>>>>
>>>>
>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>> panel-bridge driver as a module?
>>>>
>>>>
>>>> All the connectors that have been added are registered automatically
>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>> connector in the middle of setting up our driver is prone to race
>>>> conditions if the userspace decides to use them immediately.
>>>
>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>> which in the case of a panel module that creates the DSI device
>>> (adv7533-style, like you said I should use as a reference) will be after
>>> drm_mode_config_reset() and drm_dev_register().
>>
>> Okay. In the case of the msm kms driver, we defer probe until the
>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>> and drm_dev_register(). I assumed this was the general practice followed by
>> most kms drivers. I.,e the kms driver defers probe until all connector
>> related modules are inserted, and only then proceed to create a drm device.
>
> The problem, though, is the panel driver needs the MIPI DSI host to
> exist to call mipi_dsi_device_register_full() during the probe process.
> The adv7533 driver gets around this by registering the DSI device in the
> bridge attach step, but drm_panel doesn't have an attach step.
>
> Another alternative is my original version of the panel driver that was
> a mipi_dsi_device driver that registered the panel during the DSI device
> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
> phase, currently.

+ Philippe in copy because we have the same probing issue when adding
panel-bridge
with the dsi bridge

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Benjamin Gaignard]

2017-06-20 19:31 GMT+02:00 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>:
> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>
>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>
>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>> then the connector's state would never get initialized, and we'd
>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>> the connector, so that userspace can get at it.
>>>>>
>>>>
>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>> panel-bridge driver as a module?
>>>>
>>>>
>>>> All the connectors that have been added are registered automatically
>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>> connector in the middle of setting up our driver is prone to race
>>>> conditions if the userspace decides to use them immediately.
>>>
>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>> which in the case of a panel module that creates the DSI device
>>> (adv7533-style, like you said I should use as a reference) will be after
>>> drm_mode_config_reset() and drm_dev_register().
>>
>> Okay. In the case of the msm kms driver, we defer probe until the
>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>> and drm_dev_register(). I assumed this was the general practice followed by
>> most kms drivers. I.,e the kms driver defers probe until all connector
>> related modules are inserted, and only then proceed to create a drm device.
>
> The problem, though, is the panel driver needs the MIPI DSI host to
> exist to call mipi_dsi_device_register_full() during the probe process.
> The adv7533 driver gets around this by registering the DSI device in the
> bridge attach step, but drm_panel doesn't have an attach step.
>
> Another alternative is my original version of the panel driver that was
> a mipi_dsi_device driver that registered the panel during the DSI device
> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
> phase, currently.

+ Philippe in copy because we have the same probing issue when adding
panel-bridge
with the dsi bridge

>
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>> Archit Taneja <architt@codeaurora.org> writes:
>>
>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>
>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>> then the connector's state would never get initialized, and we'd
>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>> the connector, so that userspace can get at it.
>>>>>>
>>>>>
>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>> panel-bridge driver as a module?
>>>>>
>>>>>
>>>>> All the connectors that have been added are registered automatically
>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>> connector in the middle of setting up our driver is prone to race
>>>>> conditions if the userspace decides to use them immediately.
>>>>
>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>> which in the case of a panel module that creates the DSI device
>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>> drm_mode_config_reset() and drm_dev_register().
>>>
>>> Okay. In the case of the msm kms driver, we defer probe until the
>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>> and drm_dev_register(). I assumed this was the general practice followed by
>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>> related modules are inserted, and only then proceed to create a drm device.
>>
>> The problem, though, is the panel driver needs the MIPI DSI host to
>> exist to call mipi_dsi_device_register_full() during the probe process.
>> The adv7533 driver gets around this by registering the DSI device in the
>> bridge attach step, but drm_panel doesn't have an attach step.

I'm not sure how we can get around this. We had discussion about this on irc
recently, but couldn't come up with a good conclusion. We could come up with a
panel_attach() callback to make it similar to bridges, but that's just us avoiding
the real issue.

>>
>> Another alternative is my original version of the panel driver that was
>> a mipi_dsi_device driver that registered the panel during the DSI device
>> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
>> phase, currently.
> 

This would require you to have a DSI device node in DT, rather than an i2c
node, right? I don't know if we should do that because of a limitation in
our drm_mipi_dsi and drm_panel frameworks.

Does anyone have better ideas?

Thanks,
Archit

> + Philippe in copy because we have the same probing issue when adding
> panel-bridge
> with the dsi bridge
> 
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>> Archit Taneja <architt@codeaurora.org> writes:
>>
>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>
>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>> then the connector's state would never get initialized, and we'd
>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>> the connector, so that userspace can get at it.
>>>>>>
>>>>>
>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>> panel-bridge driver as a module?
>>>>>
>>>>>
>>>>> All the connectors that have been added are registered automatically
>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>> connector in the middle of setting up our driver is prone to race
>>>>> conditions if the userspace decides to use them immediately.
>>>>
>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>> which in the case of a panel module that creates the DSI device
>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>> drm_mode_config_reset() and drm_dev_register().
>>>
>>> Okay. In the case of the msm kms driver, we defer probe until the
>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>> and drm_dev_register(). I assumed this was the general practice followed by
>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>> related modules are inserted, and only then proceed to create a drm device.
>>
>> The problem, though, is the panel driver needs the MIPI DSI host to
>> exist to call mipi_dsi_device_register_full() during the probe process.
>> The adv7533 driver gets around this by registering the DSI device in the
>> bridge attach step, but drm_panel doesn't have an attach step.

I'm not sure how we can get around this. We had discussion about this on irc
recently, but couldn't come up with a good conclusion. We could come up with a
panel_attach() callback to make it similar to bridges, but that's just us avoiding
the real issue.

>>
>> Another alternative is my original version of the panel driver that was
>> a mipi_dsi_device driver that registered the panel during the DSI device
>> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
>> phase, currently.
> 

This would require you to have a DSI device node in DT, rather than an i2c
node, right? I don't know if we should do that because of a limitation in
our drm_mipi_dsi and drm_panel frameworks.

Does anyone have better ideas?

Thanks,
Archit

> + Philippe in copy because we have the same probing issue when adding
> panel-bridge
> with the dsi bridge
> 
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Thu, 22 Jun 2017 13:47:43 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
> > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:  
> >> Archit Taneja <architt@codeaurora.org> writes:
> >>  
> >>> On 06/16/2017 08:13 PM, Eric Anholt wrote:  
> >>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>  
> >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:  
> >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>> then the connector's state would never get initialized, and we'd
> >>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>> the connector, so that userspace can get at it.
> >>>>>>  
> >>>>>
> >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>> panel-bridge driver as a module?
> >>>>>
> >>>>>
> >>>>> All the connectors that have been added are registered automatically
> >>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>> connector in the middle of setting up our driver is prone to race
> >>>>> conditions if the userspace decides to use them immediately.  
> >>>>
> >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>> which in the case of a panel module that creates the DSI device
> >>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>> drm_mode_config_reset() and drm_dev_register().  
> >>>
> >>> Okay. In the case of the msm kms driver, we defer probe until the
> >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>> and drm_dev_register(). I assumed this was the general practice followed by
> >>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>> related modules are inserted, and only then proceed to create a drm device.  
> >>
> >> The problem, though, is the panel driver needs the MIPI DSI host to
> >> exist to call mipi_dsi_device_register_full() during the probe process.
> >> The adv7533 driver gets around this by registering the DSI device in the
> >> bridge attach step, but drm_panel doesn't have an attach step.  
> 
> I'm not sure how we can get around this. We had discussion about this on irc
> recently, but couldn't come up with a good conclusion. We could come up with a
> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> the real issue.

How about making DSI dev registration fully asynchronous, that is, DSI
devs declared in the DT under the DSI host node will be
registered/attached at probe time, and devs using another control bus
(like the adv7533 controller over i2c) will be registered afterwards.

That implies moving the drm_brige registration logic outside of the DSI
host ->probe() path. The idea would be to check if all devs connected
to the DSI bus are ready at dsi_host->attach() time. If they are, we
can finally register the XXX -> DSI bridge. If they're not (because
some devs connected to the DSI bus have not been probed yet), then we
do not register the drm_bridge and wait for the next dsi_host->attach()
event.

With this solution, I think we can avoid the chicken&egg problem where
the adv7533 dev is waiting for the DSI host to be probed to be able to
register a DSI dev with mipi_dsi_device_register_full() and the DSI
host needs all devs to be registered to not return -EPROBE_DEFER.

> 
> >>
> >> Another alternative is my original version of the panel driver that was
> >> a mipi_dsi_device driver that registered the panel during the DSI device
> >> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
> >> phase, currently.  
> >   
> 
> This would require you to have a DSI device node in DT, rather than an i2c
> node, right? I don't know if we should do that because of a limitation in
> our drm_mipi_dsi and drm_panel frameworks.
> 
> Does anyone have better ideas?

Well, these mixed-bus cases are really unpractical to represent in the
DT (and also not easy to deal with the device-model). I understand that
the initial solution was to put the device under the control-bus, and
not the data-bus, but then it leads to the current situation where we
don't know exactly when things are ready to be used.

Theoretically, we should wait for both the i2c and DSI ends to be
attached to their respective controller before starting using the
device. But that requires representing things with a 3rd DT node
aggregating both components:

	i2c-bus {
		...

		adv7533_i2c@xx {
			...
			adv,adv7533-master = <&adv7533>;
		};
	};

	...
	dsi-bus {
		...
		adv7533_dsi: adv7533_dsi@xx {
			...
			adv,adv7533-master = <&adv7533>;
		};
	};

	adv7533: adv7533 {
		...
		adv,adv7533-dsi = <&adv7533_dsi>;
		adv,adv7533-i2c = <&adv7533_i2c>;
	};


I guess this kind of representation has already been discussed and
rejected for good reasons (note that we could also use OF graph
representation to link adv7533 master and its components).

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Thu, 22 Jun 2017 13:47:43 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
> > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:  
> >> Archit Taneja <architt@codeaurora.org> writes:
> >>  
> >>> On 06/16/2017 08:13 PM, Eric Anholt wrote:  
> >>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>  
> >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:  
> >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>> then the connector's state would never get initialized, and we'd
> >>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>> the connector, so that userspace can get at it.
> >>>>>>  
> >>>>>
> >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>> panel-bridge driver as a module?
> >>>>>
> >>>>>
> >>>>> All the connectors that have been added are registered automatically
> >>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>> connector in the middle of setting up our driver is prone to race
> >>>>> conditions if the userspace decides to use them immediately.  
> >>>>
> >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>> which in the case of a panel module that creates the DSI device
> >>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>> drm_mode_config_reset() and drm_dev_register().  
> >>>
> >>> Okay. In the case of the msm kms driver, we defer probe until the
> >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>> and drm_dev_register(). I assumed this was the general practice followed by
> >>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>> related modules are inserted, and only then proceed to create a drm device.  
> >>
> >> The problem, though, is the panel driver needs the MIPI DSI host to
> >> exist to call mipi_dsi_device_register_full() during the probe process.
> >> The adv7533 driver gets around this by registering the DSI device in the
> >> bridge attach step, but drm_panel doesn't have an attach step.  
> 
> I'm not sure how we can get around this. We had discussion about this on irc
> recently, but couldn't come up with a good conclusion. We could come up with a
> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> the real issue.

How about making DSI dev registration fully asynchronous, that is, DSI
devs declared in the DT under the DSI host node will be
registered/attached at probe time, and devs using another control bus
(like the adv7533 controller over i2c) will be registered afterwards.

That implies moving the drm_brige registration logic outside of the DSI
host ->probe() path. The idea would be to check if all devs connected
to the DSI bus are ready at dsi_host->attach() time. If they are, we
can finally register the XXX -> DSI bridge. If they're not (because
some devs connected to the DSI bus have not been probed yet), then we
do not register the drm_bridge and wait for the next dsi_host->attach()
event.

With this solution, I think we can avoid the chicken&egg problem where
the adv7533 dev is waiting for the DSI host to be probed to be able to
register a DSI dev with mipi_dsi_device_register_full() and the DSI
host needs all devs to be registered to not return -EPROBE_DEFER.

> 
> >>
> >> Another alternative is my original version of the panel driver that was
> >> a mipi_dsi_device driver that registered the panel during the DSI device
> >> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
> >> phase, currently.  
> >   
> 
> This would require you to have a DSI device node in DT, rather than an i2c
> node, right? I don't know if we should do that because of a limitation in
> our drm_mipi_dsi and drm_panel frameworks.
> 
> Does anyone have better ideas?

Well, these mixed-bus cases are really unpractical to represent in the
DT (and also not easy to deal with the device-model). I understand that
the initial solution was to put the device under the control-bus, and
not the data-bus, but then it leads to the current situation where we
don't know exactly when things are ready to be used.

Theoretically, we should wait for both the i2c and DSI ends to be
attached to their respective controller before starting using the
device. But that requires representing things with a 3rd DT node
aggregating both components:

	i2c-bus {
		...

		adv7533_i2c@xx {
			...
			adv,adv7533-master = <&adv7533>;
		};
	};

	...
	dsi-bus {
		...
		adv7533_dsi: adv7533_dsi@xx {
			...
			adv,adv7533-master = <&adv7533>;
		};
	};

	adv7533: adv7533 {
		...
		adv,adv7533-dsi = <&adv7533_dsi>;
		adv,adv7533-i2c = <&adv7533_i2c>;
	};


I guess this kind of representation has already been discussed and
rejected for good reasons (note that we could also use OF graph
representation to link adv7533 master and its components).
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Andrzej Hajda]

On 22.06.2017 11:23, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 13:47:43 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:  
>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>  
>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:  
>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>  
>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:  
>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>  
>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>> panel-bridge driver as a module?
>>>>>>>
>>>>>>>
>>>>>>> All the connectors that have been added are registered automatically
>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>> conditions if the userspace decides to use them immediately.  
>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>> which in the case of a panel module that creates the DSI device
>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>> drm_mode_config_reset() and drm_dev_register().  
>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>> related modules are inserted, and only then proceed to create a drm device.  
>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>> bridge attach step, but drm_panel doesn't have an attach step.  
>> I'm not sure how we can get around this. We had discussion about this on irc
>> recently, but couldn't come up with a good conclusion. We could come up with a
>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>> the real issue.
> How about making DSI dev registration fully asynchronous, that is, DSI
> devs declared in the DT under the DSI host node will be
> registered/attached at probe time, and devs using another control bus
> (like the adv7533 controller over i2c) will be registered afterwards.
>
> That implies moving the drm_brige registration logic outside of the DSI
> host ->probe() path. The idea would be to check if all devs connected
> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> can finally register the XXX -> DSI bridge. If they're not (because
> some devs connected to the DSI bus have not been probed yet), then we
> do not register the drm_bridge and wait for the next dsi_host->attach()
> event.

I guess you assumes that dsi-host knows all devs connected to it, thanks to:
- subnodes of the host - ie. devices controlled via dsi bus,
- graph links from host ports/endpoints - ie. devices controlled by
other buses, for example adv7533.

I would separate both abstractions to make it more clear:
1. MIPI bus should be registered early - to allow create/bind devices on it,
2. drm_bridge should be registered only if all required sinks
(bridges/panels) are registered.

First point seems OK, I am not sure about the 2nd one - if used
consistently, it would require building pipeline from sink to source.
By the way is there any pipeline with two consecutive external bridges
in the mainline?

Regards
Andrzej

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Andrzej Hajda]

On 22.06.2017 11:23, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 13:47:43 +0530
> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>:  
>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>  
>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:  
>>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>>  
>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:  
>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>  
>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>> panel-bridge driver as a module?
>>>>>>>
>>>>>>>
>>>>>>> All the connectors that have been added are registered automatically
>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>> conditions if the userspace decides to use them immediately.  
>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>> which in the case of a panel module that creates the DSI device
>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>> drm_mode_config_reset() and drm_dev_register().  
>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>> related modules are inserted, and only then proceed to create a drm device.  
>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>> bridge attach step, but drm_panel doesn't have an attach step.  
>> I'm not sure how we can get around this. We had discussion about this on irc
>> recently, but couldn't come up with a good conclusion. We could come up with a
>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>> the real issue.
> How about making DSI dev registration fully asynchronous, that is, DSI
> devs declared in the DT under the DSI host node will be
> registered/attached at probe time, and devs using another control bus
> (like the adv7533 controller over i2c) will be registered afterwards.
>
> That implies moving the drm_brige registration logic outside of the DSI
> host ->probe() path. The idea would be to check if all devs connected
> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> can finally register the XXX -> DSI bridge. If they're not (because
> some devs connected to the DSI bus have not been probed yet), then we
> do not register the drm_bridge and wait for the next dsi_host->attach()
> event.

I guess you assumes that dsi-host knows all devs connected to it, thanks to:
- subnodes of the host - ie. devices controlled via dsi bus,
- graph links from host ports/endpoints - ie. devices controlled by
other buses, for example adv7533.

I would separate both abstractions to make it more clear:
1. MIPI bus should be registered early - to allow create/bind devices on it,
2. drm_bridge should be registered only if all required sinks
(bridges/panels) are registered.

First point seems OK, I am not sure about the 2nd one - if used
consistently, it would require building pipeline from sink to source.
By the way is there any pipeline with two consecutive external bridges
in the mainline?

Regards
Andrzej

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Thu, 22 Jun 2017 14:29:07 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 22.06.2017 11:23, Boris Brezillon wrote:
> > On Thu, 22 Jun 2017 13:47:43 +0530
> > Archit Taneja <architt@codeaurora.org> wrote:
> >  
> >> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:  
> >>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:    
> >>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>    
> >>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:    
> >>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>    
> >>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:    
> >>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>>>> then the connector's state would never get initialized, and we'd
> >>>>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>>>> the connector, so that userspace can get at it.
> >>>>>>>>    
> >>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>>>> panel-bridge driver as a module?
> >>>>>>>
> >>>>>>>
> >>>>>>> All the connectors that have been added are registered automatically
> >>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>>>> connector in the middle of setting up our driver is prone to race
> >>>>>>> conditions if the userspace decides to use them immediately.    
> >>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>>>> which in the case of a panel module that creates the DSI device
> >>>>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>>>> drm_mode_config_reset() and drm_dev_register().    
> >>>>> Okay. In the case of the msm kms driver, we defer probe until the
> >>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>>>> and drm_dev_register(). I assumed this was the general practice followed by
> >>>>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>>>> related modules are inserted, and only then proceed to create a drm device.    
> >>>> The problem, though, is the panel driver needs the MIPI DSI host to
> >>>> exist to call mipi_dsi_device_register_full() during the probe process.
> >>>> The adv7533 driver gets around this by registering the DSI device in the
> >>>> bridge attach step, but drm_panel doesn't have an attach step.    
> >> I'm not sure how we can get around this. We had discussion about this on irc
> >> recently, but couldn't come up with a good conclusion. We could come up with a
> >> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> >> the real issue.  
> > How about making DSI dev registration fully asynchronous, that is, DSI
> > devs declared in the DT under the DSI host node will be
> > registered/attached at probe time, and devs using another control bus
> > (like the adv7533 controller over i2c) will be registered afterwards.
> >
> > That implies moving the drm_brige registration logic outside of the DSI
> > host ->probe() path. The idea would be to check if all devs connected
> > to the DSI bus are ready at dsi_host->attach() time. If they are, we
> > can finally register the XXX -> DSI bridge. If they're not (because
> > some devs connected to the DSI bus have not been probed yet), then we
> > do not register the drm_bridge and wait for the next dsi_host->attach()
> > event.  
> 
> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
> - subnodes of the host - ie. devices controlled via dsi bus,
> - graph links from host ports/endpoints - ie. devices controlled by
> other buses, for example adv7533.

Yep, but I think that's already a requirement when populating devices
with the OF graph method (if one of the DSI output endpoint does not
have a drm_bridge/panel attached to it, the DSI host driver returns
-EPROBE_DEFER).

> 
> I would separate both abstractions to make it more clear:
> 1. MIPI bus should be registered early - to allow create/bind devices on it,

Exactly.

> 2. drm_bridge should be registered only if all required sinks
> (bridges/panels) are registered.

That's true, until we find a solution to support add DRM bridge hotplug.

> 
> First point seems OK, I am not sure about the 2nd one - if used
> consistently, it would require building pipeline from sink to source.

Yes.

> By the way is there any pipeline with two consecutive external bridges
> in the mainline?

I don't know if it exists in mainline, but I had to do that on my FPGA
platform when developing/testing Cadence DSI host driver. I had the
following chain and it worked just fine:

CRTC -> DPI encoder -> DPI to DSI bridge -> DSI to DPI bridge -> DPI to HDMI bridge (adv7511) -> HDMI connector

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Thu, 22 Jun 2017 14:29:07 +0200
Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:

> On 22.06.2017 11:23, Boris Brezillon wrote:
> > On Thu, 22 Jun 2017 13:47:43 +0530
> > Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> >  
> >> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:  
> >>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>:    
> >>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
> >>>>    
> >>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:    
> >>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
> >>>>>>    
> >>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:    
> >>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>>>> then the connector's state would never get initialized, and we'd
> >>>>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>>>> the connector, so that userspace can get at it.
> >>>>>>>>    
> >>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>>>> panel-bridge driver as a module?
> >>>>>>>
> >>>>>>>
> >>>>>>> All the connectors that have been added are registered automatically
> >>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>>>> connector in the middle of setting up our driver is prone to race
> >>>>>>> conditions if the userspace decides to use them immediately.    
> >>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>>>> which in the case of a panel module that creates the DSI device
> >>>>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>>>> drm_mode_config_reset() and drm_dev_register().    
> >>>>> Okay. In the case of the msm kms driver, we defer probe until the
> >>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>>>> and drm_dev_register(). I assumed this was the general practice followed by
> >>>>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>>>> related modules are inserted, and only then proceed to create a drm device.    
> >>>> The problem, though, is the panel driver needs the MIPI DSI host to
> >>>> exist to call mipi_dsi_device_register_full() during the probe process.
> >>>> The adv7533 driver gets around this by registering the DSI device in the
> >>>> bridge attach step, but drm_panel doesn't have an attach step.    
> >> I'm not sure how we can get around this. We had discussion about this on irc
> >> recently, but couldn't come up with a good conclusion. We could come up with a
> >> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> >> the real issue.  
> > How about making DSI dev registration fully asynchronous, that is, DSI
> > devs declared in the DT under the DSI host node will be
> > registered/attached at probe time, and devs using another control bus
> > (like the adv7533 controller over i2c) will be registered afterwards.
> >
> > That implies moving the drm_brige registration logic outside of the DSI
> > host ->probe() path. The idea would be to check if all devs connected
> > to the DSI bus are ready at dsi_host->attach() time. If they are, we
> > can finally register the XXX -> DSI bridge. If they're not (because
> > some devs connected to the DSI bus have not been probed yet), then we
> > do not register the drm_bridge and wait for the next dsi_host->attach()
> > event.  
> 
> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
> - subnodes of the host - ie. devices controlled via dsi bus,
> - graph links from host ports/endpoints - ie. devices controlled by
> other buses, for example adv7533.

Yep, but I think that's already a requirement when populating devices
with the OF graph method (if one of the DSI output endpoint does not
have a drm_bridge/panel attached to it, the DSI host driver returns
-EPROBE_DEFER).

> 
> I would separate both abstractions to make it more clear:
> 1. MIPI bus should be registered early - to allow create/bind devices on it,

Exactly.

> 2. drm_bridge should be registered only if all required sinks
> (bridges/panels) are registered.

That's true, until we find a solution to support add DRM bridge hotplug.

> 
> First point seems OK, I am not sure about the 2nd one - if used
> consistently, it would require building pipeline from sink to source.

Yes.

> By the way is there any pipeline with two consecutive external bridges
> in the mainline?

I don't know if it exists in mainline, but I had to do that on my FPGA
platform when developing/testing Cadence DSI host driver. I had the
following chain and it worked just fine:

CRTC -> DPI encoder -> DPI to DSI bridge -> DSI to DPI bridge -> DPI to HDMI bridge (adv7511) -> HDMI connector

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Andrzej Hajda]

On 22.06.2017 14:41, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 14:29:07 +0200
> Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> On 22.06.2017 11:23, Boris Brezillon wrote:
>>> On Thu, 22 Jun 2017 13:47:43 +0530
>>> Archit Taneja <architt@codeaurora.org> wrote:
>>>  
>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:  
>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:    
>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>    
>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:    
>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>>>    
>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:    
>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>>    
>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>>> panel-bridge driver as a module?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>>> conditions if the userspace decides to use them immediately.    
>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>>> drm_mode_config_reset() and drm_dev_register().    
>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>>> related modules are inserted, and only then proceed to create a drm device.    
>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>>> bridge attach step, but drm_panel doesn't have an attach step.    
>>>> I'm not sure how we can get around this. We had discussion about this on irc
>>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>>> the real issue.  
>>> How about making DSI dev registration fully asynchronous, that is, DSI
>>> devs declared in the DT under the DSI host node will be
>>> registered/attached at probe time, and devs using another control bus
>>> (like the adv7533 controller over i2c) will be registered afterwards.
>>>
>>> That implies moving the drm_brige registration logic outside of the DSI
>>> host ->probe() path. The idea would be to check if all devs connected
>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>>> can finally register the XXX -> DSI bridge. If they're not (because
>>> some devs connected to the DSI bus have not been probed yet), then we
>>> do not register the drm_bridge and wait for the next dsi_host->attach()
>>> event.  
>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
>> - subnodes of the host - ie. devices controlled via dsi bus,
>> - graph links from host ports/endpoints - ie. devices controlled by
>> other buses, for example adv7533.
> Yep, but I think that's already a requirement when populating devices
> with the OF graph method (if one of the DSI output endpoint does not
> have a drm_bridge/panel attached to it, the DSI host driver returns
> -EPROBE_DEFER).
>
>> I would separate both abstractions to make it more clear:
>> 1. MIPI bus should be registered early - to allow create/bind devices on it,
> Exactly.
>
>> 2. drm_bridge should be registered only if all required sinks
>> (bridges/panels) are registered.
> That's true, until we find a solution to support add DRM bridge hotplug.
>
>> First point seems OK, I am not sure about the 2nd one - if used
>> consistently, it would require building pipeline from sink to source.
> Yes.

Since drm_bridge_attach requires encoder to be not null pipeline
creation would be painful:
1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
registering bridge and cache the result for later use.
2. After encoder finds directly connected bridge, it can attach it.
3. attach callback of every bridge should attach subsequent bridge.

Quite complicated, maybe bridges should be chained w/o available
encoder, and later attached to encoder with other helper, for example
drm_bridge_chain_attach.

Regards
Andrzej

>
>> By the way is there any pipeline with two consecutive external bridges
>> in the mainline?
> I don't know if it exists in mainline, but I had to do that on my FPGA
> platform when developing/testing Cadence DSI host driver. I had the
> following chain and it worked just fine:
>
> CRTC -> DPI encoder -> DPI to DSI bridge -> DSI to DPI bridge -> DPI to HDMI bridge (adv7511) -> HDMI connector
>
>
>
>

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Andrzej Hajda]

On 22.06.2017 14:41, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 14:29:07 +0200
> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>
>> On 22.06.2017 11:23, Boris Brezillon wrote:
>>> On Thu, 22 Jun 2017 13:47:43 +0530
>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>  
>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:  
>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>:    
>>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>>    
>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:    
>>>>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>>>>    
>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:    
>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>>    
>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>>> panel-bridge driver as a module?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>>> conditions if the userspace decides to use them immediately.    
>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>>> drm_mode_config_reset() and drm_dev_register().    
>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>>> related modules are inserted, and only then proceed to create a drm device.    
>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>>> bridge attach step, but drm_panel doesn't have an attach step.    
>>>> I'm not sure how we can get around this. We had discussion about this on irc
>>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>>> the real issue.  
>>> How about making DSI dev registration fully asynchronous, that is, DSI
>>> devs declared in the DT under the DSI host node will be
>>> registered/attached at probe time, and devs using another control bus
>>> (like the adv7533 controller over i2c) will be registered afterwards.
>>>
>>> That implies moving the drm_brige registration logic outside of the DSI
>>> host ->probe() path. The idea would be to check if all devs connected
>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>>> can finally register the XXX -> DSI bridge. If they're not (because
>>> some devs connected to the DSI bus have not been probed yet), then we
>>> do not register the drm_bridge and wait for the next dsi_host->attach()
>>> event.  
>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
>> - subnodes of the host - ie. devices controlled via dsi bus,
>> - graph links from host ports/endpoints - ie. devices controlled by
>> other buses, for example adv7533.
> Yep, but I think that's already a requirement when populating devices
> with the OF graph method (if one of the DSI output endpoint does not
> have a drm_bridge/panel attached to it, the DSI host driver returns
> -EPROBE_DEFER).
>
>> I would separate both abstractions to make it more clear:
>> 1. MIPI bus should be registered early - to allow create/bind devices on it,
> Exactly.
>
>> 2. drm_bridge should be registered only if all required sinks
>> (bridges/panels) are registered.
> That's true, until we find a solution to support add DRM bridge hotplug.
>
>> First point seems OK, I am not sure about the 2nd one - if used
>> consistently, it would require building pipeline from sink to source.
> Yes.

Since drm_bridge_attach requires encoder to be not null pipeline
creation would be painful:
1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
registering bridge and cache the result for later use.
2. After encoder finds directly connected bridge, it can attach it.
3. attach callback of every bridge should attach subsequent bridge.

Quite complicated, maybe bridges should be chained w/o available
encoder, and later attached to encoder with other helper, for example
drm_bridge_chain_attach.

Regards
Andrzej

>
>> By the way is there any pipeline with two consecutive external bridges
>> in the mainline?
> I don't know if it exists in mainline, but I had to do that on my FPGA
> platform when developing/testing Cadence DSI host driver. I had the
> following chain and it worked just fine:
>
> CRTC -> DPI encoder -> DPI to DSI bridge -> DSI to DPI bridge -> DPI to HDMI bridge (adv7511) -> HDMI connector
>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Thu, 22 Jun 2017 15:16:47 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 22.06.2017 14:41, Boris Brezillon wrote:
> > On Thu, 22 Jun 2017 14:29:07 +0200
> > Andrzej Hajda <a.hajda@samsung.com> wrote:
> >  
> >> On 22.06.2017 11:23, Boris Brezillon wrote:  
> >>> On Thu, 22 Jun 2017 13:47:43 +0530
> >>> Archit Taneja <architt@codeaurora.org> wrote:
> >>>    
> >>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:    
> >>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:      
> >>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>      
> >>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:      
> >>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>>>      
> >>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:      
> >>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>>>>>> then the connector's state would never get initialized, and we'd
> >>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>>>>>> the connector, so that userspace can get at it.
> >>>>>>>>>>      
> >>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>>>>>> panel-bridge driver as a module?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> All the connectors that have been added are registered automatically
> >>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>>>>>> connector in the middle of setting up our driver is prone to race
> >>>>>>>>> conditions if the userspace decides to use them immediately.      
> >>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>>>>>> which in the case of a panel module that creates the DSI device
> >>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>>>>>> drm_mode_config_reset() and drm_dev_register().      
> >>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
> >>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
> >>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>>>>>> related modules are inserted, and only then proceed to create a drm device.      
> >>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
> >>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
> >>>>>> The adv7533 driver gets around this by registering the DSI device in the
> >>>>>> bridge attach step, but drm_panel doesn't have an attach step.      
> >>>> I'm not sure how we can get around this. We had discussion about this on irc
> >>>> recently, but couldn't come up with a good conclusion. We could come up with a
> >>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> >>>> the real issue.    
> >>> How about making DSI dev registration fully asynchronous, that is, DSI
> >>> devs declared in the DT under the DSI host node will be
> >>> registered/attached at probe time, and devs using another control bus
> >>> (like the adv7533 controller over i2c) will be registered afterwards.
> >>>
> >>> That implies moving the drm_brige registration logic outside of the DSI
> >>> host ->probe() path. The idea would be to check if all devs connected
> >>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> >>> can finally register the XXX -> DSI bridge. If they're not (because
> >>> some devs connected to the DSI bus have not been probed yet), then we
> >>> do not register the drm_bridge and wait for the next dsi_host->attach()
> >>> event.    
> >> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
> >> - subnodes of the host - ie. devices controlled via dsi bus,
> >> - graph links from host ports/endpoints - ie. devices controlled by
> >> other buses, for example adv7533.  
> > Yep, but I think that's already a requirement when populating devices
> > with the OF graph method (if one of the DSI output endpoint does not
> > have a drm_bridge/panel attached to it, the DSI host driver returns
> > -EPROBE_DEFER).
> >  
> >> I would separate both abstractions to make it more clear:
> >> 1. MIPI bus should be registered early - to allow create/bind devices on it,  
> > Exactly.
> >  
> >> 2. drm_bridge should be registered only if all required sinks
> >> (bridges/panels) are registered.  
> > That's true, until we find a solution to support add DRM bridge hotplug.
> >  
> >> First point seems OK, I am not sure about the 2nd one - if used
> >> consistently, it would require building pipeline from sink to source.  
> > Yes.  
> 
> Since drm_bridge_attach requires encoder to be not null pipeline
> creation would be painful:
> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
> registering bridge and cache the result for later use.

Shouldn't be hard to do since dsi_host->attach() is called each time a
sink is added (and ready to use). All you need to do is retrieve the
bridge pointer and put it in a list embedded in the DSI host priv
struct. Once you have all sinks added to this list (can be checked by
counting the number of endpoints and DSI devs at probe time), you can
register the DSI bridge and wait for someone to call ->attach() on it.

In the ->attach() hook of the DSI bridge, you'll have to attach all
sinks stored in the list to the DSI bridge. Note that right now you have
a 1:1 relationship, which prevents you from having one DSI bridge that
can attach to different bridges available on the DSI bus (e.g. DSI ->
HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).

> 2. After encoder finds directly connected bridge, it can attach it.

I don't get that one.

> 3. attach callback of every bridge should attach subsequent bridge.

Yep.

> 
> Quite complicated,

Not sure it's more complicated than what we have right now.

> maybe bridges should be chained w/o available
> encoder, and later attached to encoder with other helper, for example
> drm_bridge_chain_attach.

That's also a solution.

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Thu, 22 Jun 2017 15:16:47 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 22.06.2017 14:41, Boris Brezillon wrote:
> > On Thu, 22 Jun 2017 14:29:07 +0200
> > Andrzej Hajda <a.hajda@samsung.com> wrote:
> >  
> >> On 22.06.2017 11:23, Boris Brezillon wrote:  
> >>> On Thu, 22 Jun 2017 13:47:43 +0530
> >>> Archit Taneja <architt@codeaurora.org> wrote:
> >>>    
> >>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:    
> >>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:      
> >>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>      
> >>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:      
> >>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>>>      
> >>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:      
> >>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>>>>>> then the connector's state would never get initialized, and we'd
> >>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>>>>>> the connector, so that userspace can get at it.
> >>>>>>>>>>      
> >>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>>>>>> panel-bridge driver as a module?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> All the connectors that have been added are registered automatically
> >>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>>>>>> connector in the middle of setting up our driver is prone to race
> >>>>>>>>> conditions if the userspace decides to use them immediately.      
> >>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>>>>>> which in the case of a panel module that creates the DSI device
> >>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>>>>>> drm_mode_config_reset() and drm_dev_register().      
> >>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
> >>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
> >>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>>>>>> related modules are inserted, and only then proceed to create a drm device.      
> >>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
> >>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
> >>>>>> The adv7533 driver gets around this by registering the DSI device in the
> >>>>>> bridge attach step, but drm_panel doesn't have an attach step.      
> >>>> I'm not sure how we can get around this. We had discussion about this on irc
> >>>> recently, but couldn't come up with a good conclusion. We could come up with a
> >>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> >>>> the real issue.    
> >>> How about making DSI dev registration fully asynchronous, that is, DSI
> >>> devs declared in the DT under the DSI host node will be
> >>> registered/attached at probe time, and devs using another control bus
> >>> (like the adv7533 controller over i2c) will be registered afterwards.
> >>>
> >>> That implies moving the drm_brige registration logic outside of the DSI
> >>> host ->probe() path. The idea would be to check if all devs connected
> >>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> >>> can finally register the XXX -> DSI bridge. If they're not (because
> >>> some devs connected to the DSI bus have not been probed yet), then we
> >>> do not register the drm_bridge and wait for the next dsi_host->attach()
> >>> event.    
> >> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
> >> - subnodes of the host - ie. devices controlled via dsi bus,
> >> - graph links from host ports/endpoints - ie. devices controlled by
> >> other buses, for example adv7533.  
> > Yep, but I think that's already a requirement when populating devices
> > with the OF graph method (if one of the DSI output endpoint does not
> > have a drm_bridge/panel attached to it, the DSI host driver returns
> > -EPROBE_DEFER).
> >  
> >> I would separate both abstractions to make it more clear:
> >> 1. MIPI bus should be registered early - to allow create/bind devices on it,  
> > Exactly.
> >  
> >> 2. drm_bridge should be registered only if all required sinks
> >> (bridges/panels) are registered.  
> > That's true, until we find a solution to support add DRM bridge hotplug.
> >  
> >> First point seems OK, I am not sure about the 2nd one - if used
> >> consistently, it would require building pipeline from sink to source.  
> > Yes.  
> 
> Since drm_bridge_attach requires encoder to be not null pipeline
> creation would be painful:
> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
> registering bridge and cache the result for later use.

Shouldn't be hard to do since dsi_host->attach() is called each time a
sink is added (and ready to use). All you need to do is retrieve the
bridge pointer and put it in a list embedded in the DSI host priv
struct. Once you have all sinks added to this list (can be checked by
counting the number of endpoints and DSI devs at probe time), you can
register the DSI bridge and wait for someone to call ->attach() on it.

In the ->attach() hook of the DSI bridge, you'll have to attach all
sinks stored in the list to the DSI bridge. Note that right now you have
a 1:1 relationship, which prevents you from having one DSI bridge that
can attach to different bridges available on the DSI bus (e.g. DSI ->
HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).

> 2. After encoder finds directly connected bridge, it can attach it.

I don't get that one.

> 3. attach callback of every bridge should attach subsequent bridge.

Yep.

> 
> Quite complicated,

Not sure it's more complicated than what we have right now.

> maybe bridges should be chained w/o available
> encoder, and later attached to encoder with other helper, for example
> drm_bridge_chain_attach.

That's also a solution.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Andrzej Hajda]

On 22.06.2017 15:34, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 15:16:47 +0200
> Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> On 22.06.2017 14:41, Boris Brezillon wrote:
>>> On Thu, 22 Jun 2017 14:29:07 +0200
>>> Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>  
>>>> On 22.06.2017 11:23, Boris Brezillon wrote:  
>>>>> On Thu, 22 Jun 2017 13:47:43 +0530
>>>>> Archit Taneja <architt@codeaurora.org> wrote:
>>>>>    
>>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:    
>>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:      
>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>>>      
>>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:      
>>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>>>>>      
>>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:      
>>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>>>>      
>>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>>>>> panel-bridge driver as a module?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>>>>> conditions if the userspace decides to use them immediately.      
>>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>>>>> drm_mode_config_reset() and drm_dev_register().      
>>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>>>>> related modules are inserted, and only then proceed to create a drm device.      
>>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>>>>> bridge attach step, but drm_panel doesn't have an attach step.      
>>>>>> I'm not sure how we can get around this. We had discussion about this on irc
>>>>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>>>>> the real issue.    
>>>>> How about making DSI dev registration fully asynchronous, that is, DSI
>>>>> devs declared in the DT under the DSI host node will be
>>>>> registered/attached at probe time, and devs using another control bus
>>>>> (like the adv7533 controller over i2c) will be registered afterwards.
>>>>>
>>>>> That implies moving the drm_brige registration logic outside of the DSI
>>>>> host ->probe() path. The idea would be to check if all devs connected
>>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>>>>> can finally register the XXX -> DSI bridge. If they're not (because
>>>>> some devs connected to the DSI bus have not been probed yet), then we
>>>>> do not register the drm_bridge and wait for the next dsi_host->attach()
>>>>> event.    
>>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
>>>> - subnodes of the host - ie. devices controlled via dsi bus,
>>>> - graph links from host ports/endpoints - ie. devices controlled by
>>>> other buses, for example adv7533.  
>>> Yep, but I think that's already a requirement when populating devices
>>> with the OF graph method (if one of the DSI output endpoint does not
>>> have a drm_bridge/panel attached to it, the DSI host driver returns
>>> -EPROBE_DEFER).
>>>  
>>>> I would separate both abstractions to make it more clear:
>>>> 1. MIPI bus should be registered early - to allow create/bind devices on it,  
>>> Exactly.
>>>  
>>>> 2. drm_bridge should be registered only if all required sinks
>>>> (bridges/panels) are registered.  
>>> That's true, until we find a solution to support add DRM bridge hotplug.
>>>  
>>>> First point seems OK, I am not sure about the 2nd one - if used
>>>> consistently, it would require building pipeline from sink to source.  
>>> Yes.  
>> Since drm_bridge_attach requires encoder to be not null pipeline
>> creation would be painful:
>> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
>> registering bridge and cache the result for later use.
> Shouldn't be hard to do since dsi_host->attach() is called each time a
> sink is added (and ready to use). All you need to do is retrieve the
> bridge pointer and put it in a list embedded in the DSI host priv
> struct. Once you have all sinks added to this list (can be checked by
> counting the number of endpoints and DSI devs at probe time), you can
> register the DSI bridge and wait for someone to call ->attach() on it.
>
> In the ->attach() hook of the DSI bridge, you'll have to attach all
> sinks stored in the list to the DSI bridge. Note that right now you have
> a 1:1 relationship, which prevents you from having one DSI bridge that
> can attach to different bridges available on the DSI bus (e.g. DSI ->
> HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).
>
>> 2. After encoder finds directly connected bridge, it can attach it.
> I don't get that one.

If you have pipeline:

crtc -> encoder -> bridge1 -> bridge2 -> panel

encoder knows only about bridge1, and must wait till it is registered,
before attaching it,
and assuming bridge must wait for its sinks before registration the
whole pipeline construction will look like:

0. encoder waits for bridge1, bridge1 waits for bridge2, bridge2 waits
for panel, usually by deferring.
1. panel is registered.
2. bridge2 finds panel and is registered.
3. bridge1 finds bridge2 and is registered.
4. encoder finds bridge1 and attach it to encoder,
4a. bridge1->attach() attach bridge2 to encoder after bridge1
4b. bridge2->attach() attach panel to bridge2

This is why it seems for me quite complicated.

Regards
Andrzej

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Andrzej Hajda]

On 22.06.2017 15:34, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 15:16:47 +0200
> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>
>> On 22.06.2017 14:41, Boris Brezillon wrote:
>>> On Thu, 22 Jun 2017 14:29:07 +0200
>>> Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>>  
>>>> On 22.06.2017 11:23, Boris Brezillon wrote:  
>>>>> On Thu, 22 Jun 2017 13:47:43 +0530
>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>>>    
>>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:    
>>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>:      
>>>>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>>>>      
>>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:      
>>>>>>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>>>>>>      
>>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:      
>>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>>>>      
>>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>>>>> panel-bridge driver as a module?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>>>>> conditions if the userspace decides to use them immediately.      
>>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>>>>> drm_mode_config_reset() and drm_dev_register().      
>>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>>>>> related modules are inserted, and only then proceed to create a drm device.      
>>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>>>>> bridge attach step, but drm_panel doesn't have an attach step.      
>>>>>> I'm not sure how we can get around this. We had discussion about this on irc
>>>>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>>>>> the real issue.    
>>>>> How about making DSI dev registration fully asynchronous, that is, DSI
>>>>> devs declared in the DT under the DSI host node will be
>>>>> registered/attached at probe time, and devs using another control bus
>>>>> (like the adv7533 controller over i2c) will be registered afterwards.
>>>>>
>>>>> That implies moving the drm_brige registration logic outside of the DSI
>>>>> host ->probe() path. The idea would be to check if all devs connected
>>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>>>>> can finally register the XXX -> DSI bridge. If they're not (because
>>>>> some devs connected to the DSI bus have not been probed yet), then we
>>>>> do not register the drm_bridge and wait for the next dsi_host->attach()
>>>>> event.    
>>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
>>>> - subnodes of the host - ie. devices controlled via dsi bus,
>>>> - graph links from host ports/endpoints - ie. devices controlled by
>>>> other buses, for example adv7533.  
>>> Yep, but I think that's already a requirement when populating devices
>>> with the OF graph method (if one of the DSI output endpoint does not
>>> have a drm_bridge/panel attached to it, the DSI host driver returns
>>> -EPROBE_DEFER).
>>>  
>>>> I would separate both abstractions to make it more clear:
>>>> 1. MIPI bus should be registered early - to allow create/bind devices on it,  
>>> Exactly.
>>>  
>>>> 2. drm_bridge should be registered only if all required sinks
>>>> (bridges/panels) are registered.  
>>> That's true, until we find a solution to support add DRM bridge hotplug.
>>>  
>>>> First point seems OK, I am not sure about the 2nd one - if used
>>>> consistently, it would require building pipeline from sink to source.  
>>> Yes.  
>> Since drm_bridge_attach requires encoder to be not null pipeline
>> creation would be painful:
>> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
>> registering bridge and cache the result for later use.
> Shouldn't be hard to do since dsi_host->attach() is called each time a
> sink is added (and ready to use). All you need to do is retrieve the
> bridge pointer and put it in a list embedded in the DSI host priv
> struct. Once you have all sinks added to this list (can be checked by
> counting the number of endpoints and DSI devs at probe time), you can
> register the DSI bridge and wait for someone to call ->attach() on it.
>
> In the ->attach() hook of the DSI bridge, you'll have to attach all
> sinks stored in the list to the DSI bridge. Note that right now you have
> a 1:1 relationship, which prevents you from having one DSI bridge that
> can attach to different bridges available on the DSI bus (e.g. DSI ->
> HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).
>
>> 2. After encoder finds directly connected bridge, it can attach it.
> I don't get that one.

If you have pipeline:

crtc -> encoder -> bridge1 -> bridge2 -> panel

encoder knows only about bridge1, and must wait till it is registered,
before attaching it,
and assuming bridge must wait for its sinks before registration the
whole pipeline construction will look like:

0. encoder waits for bridge1, bridge1 waits for bridge2, bridge2 waits
for panel, usually by deferring.
1. panel is registered.
2. bridge2 finds panel and is registered.
3. bridge1 finds bridge2 and is registered.
4. encoder finds bridge1 and attach it to encoder,
4a. bridge1->attach() attach bridge2 to encoder after bridge1
4b. bridge2->attach() attach panel to bridge2

This is why it seems for me quite complicated.

Regards
Andrzej

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Fri, 23 Jun 2017 09:22:15 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 22.06.2017 15:34, Boris Brezillon wrote:
> > On Thu, 22 Jun 2017 15:16:47 +0200
> > Andrzej Hajda <a.hajda@samsung.com> wrote:
> >  
> >> On 22.06.2017 14:41, Boris Brezillon wrote:  
> >>> On Thu, 22 Jun 2017 14:29:07 +0200
> >>> Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>>    
> >>>> On 22.06.2017 11:23, Boris Brezillon wrote:    
> >>>>> On Thu, 22 Jun 2017 13:47:43 +0530
> >>>>> Archit Taneja <architt@codeaurora.org> wrote:
> >>>>>      
> >>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:      
> >>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:        
> >>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>>>        
> >>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:        
> >>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>>>>>        
> >>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:        
> >>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>>>>>>>> then the connector's state would never get initialized, and we'd
> >>>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>>>>>>>> the connector, so that userspace can get at it.
> >>>>>>>>>>>>        
> >>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>>>>>>>> panel-bridge driver as a module?
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> All the connectors that have been added are registered automatically
> >>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>>>>>>>> connector in the middle of setting up our driver is prone to race
> >>>>>>>>>>> conditions if the userspace decides to use them immediately.        
> >>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>>>>>>>> which in the case of a panel module that creates the DSI device
> >>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>>>>>>>> drm_mode_config_reset() and drm_dev_register().        
> >>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
> >>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
> >>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>>>>>>>> related modules are inserted, and only then proceed to create a drm device.        
> >>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
> >>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
> >>>>>>>> The adv7533 driver gets around this by registering the DSI device in the
> >>>>>>>> bridge attach step, but drm_panel doesn't have an attach step.        
> >>>>>> I'm not sure how we can get around this. We had discussion about this on irc
> >>>>>> recently, but couldn't come up with a good conclusion. We could come up with a
> >>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> >>>>>> the real issue.      
> >>>>> How about making DSI dev registration fully asynchronous, that is, DSI
> >>>>> devs declared in the DT under the DSI host node will be
> >>>>> registered/attached at probe time, and devs using another control bus
> >>>>> (like the adv7533 controller over i2c) will be registered afterwards.
> >>>>>
> >>>>> That implies moving the drm_brige registration logic outside of the DSI
> >>>>> host ->probe() path. The idea would be to check if all devs connected
> >>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> >>>>> can finally register the XXX -> DSI bridge. If they're not (because
> >>>>> some devs connected to the DSI bus have not been probed yet), then we
> >>>>> do not register the drm_bridge and wait for the next dsi_host->attach()
> >>>>> event.      
> >>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
> >>>> - subnodes of the host - ie. devices controlled via dsi bus,
> >>>> - graph links from host ports/endpoints - ie. devices controlled by
> >>>> other buses, for example adv7533.    
> >>> Yep, but I think that's already a requirement when populating devices
> >>> with the OF graph method (if one of the DSI output endpoint does not
> >>> have a drm_bridge/panel attached to it, the DSI host driver returns
> >>> -EPROBE_DEFER).
> >>>    
> >>>> I would separate both abstractions to make it more clear:
> >>>> 1. MIPI bus should be registered early - to allow create/bind devices on it,    
> >>> Exactly.
> >>>    
> >>>> 2. drm_bridge should be registered only if all required sinks
> >>>> (bridges/panels) are registered.    
> >>> That's true, until we find a solution to support add DRM bridge hotplug.
> >>>    
> >>>> First point seems OK, I am not sure about the 2nd one - if used
> >>>> consistently, it would require building pipeline from sink to source.    
> >>> Yes.    
> >> Since drm_bridge_attach requires encoder to be not null pipeline
> >> creation would be painful:
> >> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
> >> registering bridge and cache the result for later use.  
> > Shouldn't be hard to do since dsi_host->attach() is called each time a
> > sink is added (and ready to use). All you need to do is retrieve the
> > bridge pointer and put it in a list embedded in the DSI host priv
> > struct. Once you have all sinks added to this list (can be checked by
> > counting the number of endpoints and DSI devs at probe time), you can
> > register the DSI bridge and wait for someone to call ->attach() on it.
> >
> > In the ->attach() hook of the DSI bridge, you'll have to attach all
> > sinks stored in the list to the DSI bridge. Note that right now you have
> > a 1:1 relationship, which prevents you from having one DSI bridge that
> > can attach to different bridges available on the DSI bus (e.g. DSI ->
> > HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).
> >  
> >> 2. After encoder finds directly connected bridge, it can attach it.  
> > I don't get that one.  
> 
> If you have pipeline:
> 
> crtc -> encoder -> bridge1 -> bridge2 -> panel
> 
> encoder knows only about bridge1, and must wait till it is registered,
> before attaching it,
> and assuming bridge must wait for its sinks before registration the
> whole pipeline construction will look like:
> 
> 0. encoder waits for bridge1, bridge1 waits for bridge2, bridge2 waits
> for panel, usually by deferring.
> 1. panel is registered.
> 2. bridge2 finds panel and is registered.
> 3. bridge1 finds bridge2 and is registered.
> 4. encoder finds bridge1 and attach it to encoder,
> 4a. bridge1->attach() attach bridge2 to encoder after bridge1
> 4b. bridge2->attach() attach panel to bridge2
> 
> This is why it seems for me quite complicated.

But that's already what happens in most drivers today (probably because
things were designed before connector hotplug was supported). I agree,
it's far from ideal, but until we get full-hotplug support, we'll have
to rely on such hacks.

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Boris Brezillon]

On Fri, 23 Jun 2017 09:22:15 +0200
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 22.06.2017 15:34, Boris Brezillon wrote:
> > On Thu, 22 Jun 2017 15:16:47 +0200
> > Andrzej Hajda <a.hajda@samsung.com> wrote:
> >  
> >> On 22.06.2017 14:41, Boris Brezillon wrote:  
> >>> On Thu, 22 Jun 2017 14:29:07 +0200
> >>> Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>>    
> >>>> On 22.06.2017 11:23, Boris Brezillon wrote:    
> >>>>> On Thu, 22 Jun 2017 13:47:43 +0530
> >>>>> Archit Taneja <architt@codeaurora.org> wrote:
> >>>>>      
> >>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:      
> >>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:        
> >>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>>>        
> >>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:        
> >>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
> >>>>>>>>>>        
> >>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:        
> >>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
> >>>>>>>>>>>> then the connector's state would never get initialized, and we'd
> >>>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
> >>>>>>>>>>>> the connector, so that userspace can get at it.
> >>>>>>>>>>>>        
> >>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
> >>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
> >>>>>>>>>>> panel-bridge driver as a module?
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> All the connectors that have been added are registered automatically
> >>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
> >>>>>>>>>>> connector in the middle of setting up our driver is prone to race
> >>>>>>>>>>> conditions if the userspace decides to use them immediately.        
> >>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
> >>>>>>>>>> which in the case of a panel module that creates the DSI device
> >>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
> >>>>>>>>>> drm_mode_config_reset() and drm_dev_register().        
> >>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
> >>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
> >>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
> >>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
> >>>>>>>>> related modules are inserted, and only then proceed to create a drm device.        
> >>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
> >>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
> >>>>>>>> The adv7533 driver gets around this by registering the DSI device in the
> >>>>>>>> bridge attach step, but drm_panel doesn't have an attach step.        
> >>>>>> I'm not sure how we can get around this. We had discussion about this on irc
> >>>>>> recently, but couldn't come up with a good conclusion. We could come up with a
> >>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> >>>>>> the real issue.      
> >>>>> How about making DSI dev registration fully asynchronous, that is, DSI
> >>>>> devs declared in the DT under the DSI host node will be
> >>>>> registered/attached at probe time, and devs using another control bus
> >>>>> (like the adv7533 controller over i2c) will be registered afterwards.
> >>>>>
> >>>>> That implies moving the drm_brige registration logic outside of the DSI
> >>>>> host ->probe() path. The idea would be to check if all devs connected
> >>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> >>>>> can finally register the XXX -> DSI bridge. If they're not (because
> >>>>> some devs connected to the DSI bus have not been probed yet), then we
> >>>>> do not register the drm_bridge and wait for the next dsi_host->attach()
> >>>>> event.      
> >>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
> >>>> - subnodes of the host - ie. devices controlled via dsi bus,
> >>>> - graph links from host ports/endpoints - ie. devices controlled by
> >>>> other buses, for example adv7533.    
> >>> Yep, but I think that's already a requirement when populating devices
> >>> with the OF graph method (if one of the DSI output endpoint does not
> >>> have a drm_bridge/panel attached to it, the DSI host driver returns
> >>> -EPROBE_DEFER).
> >>>    
> >>>> I would separate both abstractions to make it more clear:
> >>>> 1. MIPI bus should be registered early - to allow create/bind devices on it,    
> >>> Exactly.
> >>>    
> >>>> 2. drm_bridge should be registered only if all required sinks
> >>>> (bridges/panels) are registered.    
> >>> That's true, until we find a solution to support add DRM bridge hotplug.
> >>>    
> >>>> First point seems OK, I am not sure about the 2nd one - if used
> >>>> consistently, it would require building pipeline from sink to source.    
> >>> Yes.    
> >> Since drm_bridge_attach requires encoder to be not null pipeline
> >> creation would be painful:
> >> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
> >> registering bridge and cache the result for later use.  
> > Shouldn't be hard to do since dsi_host->attach() is called each time a
> > sink is added (and ready to use). All you need to do is retrieve the
> > bridge pointer and put it in a list embedded in the DSI host priv
> > struct. Once you have all sinks added to this list (can be checked by
> > counting the number of endpoints and DSI devs at probe time), you can
> > register the DSI bridge and wait for someone to call ->attach() on it.
> >
> > In the ->attach() hook of the DSI bridge, you'll have to attach all
> > sinks stored in the list to the DSI bridge. Note that right now you have
> > a 1:1 relationship, which prevents you from having one DSI bridge that
> > can attach to different bridges available on the DSI bus (e.g. DSI ->
> > HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).
> >  
> >> 2. After encoder finds directly connected bridge, it can attach it.  
> > I don't get that one.  
> 
> If you have pipeline:
> 
> crtc -> encoder -> bridge1 -> bridge2 -> panel
> 
> encoder knows only about bridge1, and must wait till it is registered,
> before attaching it,
> and assuming bridge must wait for its sinks before registration the
> whole pipeline construction will look like:
> 
> 0. encoder waits for bridge1, bridge1 waits for bridge2, bridge2 waits
> for panel, usually by deferring.
> 1. panel is registered.
> 2. bridge2 finds panel and is registered.
> 3. bridge1 finds bridge2 and is registered.
> 4. encoder finds bridge1 and attach it to encoder,
> 4a. bridge1->attach() attach bridge2 to encoder after bridge1
> 4b. bridge2->attach() attach panel to bridge2
> 
> This is why it seems for me quite complicated.

But that's already what happens in most drivers today (probably because
things were designed before connector hotplug was supported). I agree,
it's far from ideal, but until we get full-hotplug support, we'll have
to rely on such hacks.

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 6/22/2017 7:04 PM, Boris Brezillon wrote:
> On Thu, 22 Jun 2017 15:16:47 +0200
> Andrzej Hajda <a.hajda@samsung.com> wrote:
> 
>> On 22.06.2017 14:41, Boris Brezillon wrote:
>>> On Thu, 22 Jun 2017 14:29:07 +0200
>>> Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>   
>>>> On 22.06.2017 11:23, Boris Brezillon wrote:
>>>>> On Thu, 22 Jun 2017 13:47:43 +0530
>>>>> Archit Taneja <architt@codeaurora.org> wrote:
>>>>>     
>>>>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>>>>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>>>       
>>>>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>>>>>       
>>>>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>>>>       
>>>>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>>>>> panel-bridge driver as a module?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>>>>> conditions if the userspace decides to use them immediately.
>>>>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>>>>> related modules are inserted, and only then proceed to create a drm device.
>>>>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>>>>> bridge attach step, but drm_panel doesn't have an attach step.
>>>>>> I'm not sure how we can get around this. We had discussion about this on irc
>>>>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>>>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>>>>> the real issue.
>>>>> How about making DSI dev registration fully asynchronous, that is, DSI
>>>>> devs declared in the DT under the DSI host node will be
>>>>> registered/attached at probe time, and devs using another control bus
>>>>> (like the adv7533 controller over i2c) will be registered afterwards.
>>>>>
>>>>> That implies moving the drm_brige registration logic outside of the DSI
>>>>> host ->probe() path. The idea would be to check if all devs connected
>>>>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>>>>> can finally register the XXX -> DSI bridge. If they're not (because
>>>>> some devs connected to the DSI bus have not been probed yet), then we
>>>>> do not register the drm_bridge and wait for the next dsi_host->attach()
>>>>> event.
>>>> I guess you assumes that dsi-host knows all devs connected to it, thanks to:
>>>> - subnodes of the host - ie. devices controlled via dsi bus,
>>>> - graph links from host ports/endpoints - ie. devices controlled by
>>>> other buses, for example adv7533.
>>> Yep, but I think that's already a requirement when populating devices
>>> with the OF graph method (if one of the DSI output endpoint does not
>>> have a drm_bridge/panel attached to it, the DSI host driver returns
>>> -EPROBE_DEFER).
>>>   
>>>> I would separate both abstractions to make it more clear:
>>>> 1. MIPI bus should be registered early - to allow create/bind devices on it,
>>> Exactly.
>>>   
>>>> 2. drm_bridge should be registered only if all required sinks
>>>> (bridges/panels) are registered.
>>> That's true, until we find a solution to support add DRM bridge hotplug.
>>>   
>>>> First point seems OK, I am not sure about the 2nd one - if used
>>>> consistently, it would require building pipeline from sink to source.
>>> Yes.
>>
>> Since drm_bridge_attach requires encoder to be not null pipeline
>> creation would be painful:
>> 1. Every driver must call drm_of_find_panel_or_bridge on sink(s) before
>> registering bridge and cache the result for later use.
> 
> Shouldn't be hard to do since dsi_host->attach() is called each time a
> sink is added (and ready to use). All you need to do is retrieve the
> bridge pointer and put it in a list embedded in the DSI host priv
> struct. Once you have all sinks added to this list (can be checked by
> counting the number of endpoints and DSI devs at probe time), you can
> register the DSI bridge and wait for someone to call ->attach() on it.

Some of the kms drivers (including vc4) currently do the bridge
registration, and call drm_bridge_attach() in the DSI host driver's
probe path itself. I guess some of them would require some restructuring
to work with the above approach.

Other kms drivers (msm for example) already have a separate modeset_init
path where different drm objects (crtcs, encoders, bridges) are created.
That's the one that can defer rather than defer-ing the DSI host driver.

We'd want people be willing to convert their kms drivers to go ahead
with the approach.

Thanks for continuing the discussion, it was quite informative.

Regards,
Archit

> 
> In the ->attach() hook of the DSI bridge, you'll have to attach all
> sinks stored in the list to the DSI bridge. Note that right now you have
> a 1:1 relationship, which prevents you from having one DSI bridge that
> can attach to different bridges available on the DSI bus (e.g. DSI ->
> HDMI bridge on channel 0 and DSI -> LVDS bridge on channel 1).
> 
>> 2. After encoder finds directly connected bridge, it can attach it.
> 
> I don't get that one.
> 
>> 3. attach callback of every bridge should attach subsequent bridge.
> 
> Yep.
> 
>>
>> Quite complicated,
> 
> Not sure it's more complicated than what we have right now.
> 
>> maybe bridges should be chained w/o available
>> encoder, and later attached to encoder with other helper, for example
>> drm_bridge_chain_attach.
> 
> That's also a solution.
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> On Thu, 22 Jun 2017 13:47:43 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>> > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:  
>> >> Archit Taneja <architt@codeaurora.org> writes:
>> >>  
>> >>> On 06/16/2017 08:13 PM, Eric Anholt wrote:  
>> >>>> Archit Taneja <architt@codeaurora.org> writes:
>> >>>>  
>> >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:  
>> >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>> >>>>>> then the connector's state would never get initialized, and we'd
>> >>>>>> dereference the NULL in the hotplug path.  We also need to register
>> >>>>>> the connector, so that userspace can get at it.
>> >>>>>>  
>> >>>>>
>> >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>> >>>>> drm_mode_config_reset? Is it the case when we're inserting the
>> >>>>> panel-bridge driver as a module?
>> >>>>>
>> >>>>>
>> >>>>> All the connectors that have been added are registered automatically
>> >>>>> when drm_dev_register() is called by the KMS driver. Registering a
>> >>>>> connector in the middle of setting up our driver is prone to race
>> >>>>> conditions if the userspace decides to use them immediately.  
>> >>>>
>> >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>> >>>> which in the case of a panel module that creates the DSI device
>> >>>> (adv7533-style, like you said I should use as a reference) will be after
>> >>>> drm_mode_config_reset() and drm_dev_register().  
>> >>>
>> >>> Okay. In the case of the msm kms driver, we defer probe until the
>> >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>> >>> and drm_dev_register(). I assumed this was the general practice followed by
>> >>> most kms drivers. I.,e the kms driver defers probe until all connector
>> >>> related modules are inserted, and only then proceed to create a drm device.  
>> >>
>> >> The problem, though, is the panel driver needs the MIPI DSI host to
>> >> exist to call mipi_dsi_device_register_full() during the probe process.
>> >> The adv7533 driver gets around this by registering the DSI device in the
>> >> bridge attach step, but drm_panel doesn't have an attach step.  
>> 
>> I'm not sure how we can get around this. We had discussion about this on irc
>> recently, but couldn't come up with a good conclusion. We could come up with a
>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>> the real issue.
>
> How about making DSI dev registration fully asynchronous, that is, DSI
> devs declared in the DT under the DSI host node will be
> registered/attached at probe time, and devs using another control bus
> (like the adv7533 controller over i2c) will be registered afterwards.
>
> That implies moving the drm_brige registration logic outside of the DSI
> host ->probe() path. The idea would be to check if all devs connected
> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> can finally register the XXX -> DSI bridge. If they're not (because
> some devs connected to the DSI bus have not been probed yet), then we
> do not register the drm_bridge and wait for the next dsi_host->attach()
> event.
>
> With this solution, I think we can avoid the chicken&egg problem where
> the adv7533 dev is waiting for the DSI host to be probed to be able to
> register a DSI dev with mipi_dsi_device_register_full() and the DSI
> host needs all devs to be registered to not return -EPROBE_DEFER.

I've now tried having mipi_dsi_device_register_full() succeed early and
just do the device-add part when the host shows up.  The problem is
there's still mipi_dsi_attach(), which needs to be delayed until the
panel driver fills in the rest of mipi_dsi_device's fields.  Why aren't
those part of the info?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

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

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> On Thu, 22 Jun 2017 13:47:43 +0530
> Archit Taneja <architt@codeaurora.org> wrote:
>
>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>> > 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:  
>> >> Archit Taneja <architt@codeaurora.org> writes:
>> >>  
>> >>> On 06/16/2017 08:13 PM, Eric Anholt wrote:  
>> >>>> Archit Taneja <architt@codeaurora.org> writes:
>> >>>>  
>> >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:  
>> >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>> >>>>>> then the connector's state would never get initialized, and we'd
>> >>>>>> dereference the NULL in the hotplug path.  We also need to register
>> >>>>>> the connector, so that userspace can get at it.
>> >>>>>>  
>> >>>>>
>> >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>> >>>>> drm_mode_config_reset? Is it the case when we're inserting the
>> >>>>> panel-bridge driver as a module?
>> >>>>>
>> >>>>>
>> >>>>> All the connectors that have been added are registered automatically
>> >>>>> when drm_dev_register() is called by the KMS driver. Registering a
>> >>>>> connector in the middle of setting up our driver is prone to race
>> >>>>> conditions if the userspace decides to use them immediately.  
>> >>>>
>> >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>> >>>> which in the case of a panel module that creates the DSI device
>> >>>> (adv7533-style, like you said I should use as a reference) will be after
>> >>>> drm_mode_config_reset() and drm_dev_register().  
>> >>>
>> >>> Okay. In the case of the msm kms driver, we defer probe until the
>> >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>> >>> and drm_dev_register(). I assumed this was the general practice followed by
>> >>> most kms drivers. I.,e the kms driver defers probe until all connector
>> >>> related modules are inserted, and only then proceed to create a drm device.  
>> >>
>> >> The problem, though, is the panel driver needs the MIPI DSI host to
>> >> exist to call mipi_dsi_device_register_full() during the probe process.
>> >> The adv7533 driver gets around this by registering the DSI device in the
>> >> bridge attach step, but drm_panel doesn't have an attach step.  
>> 
>> I'm not sure how we can get around this. We had discussion about this on irc
>> recently, but couldn't come up with a good conclusion. We could come up with a
>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>> the real issue.
>
> How about making DSI dev registration fully asynchronous, that is, DSI
> devs declared in the DT under the DSI host node will be
> registered/attached at probe time, and devs using another control bus
> (like the adv7533 controller over i2c) will be registered afterwards.
>
> That implies moving the drm_brige registration logic outside of the DSI
> host ->probe() path. The idea would be to check if all devs connected
> to the DSI bus are ready at dsi_host->attach() time. If they are, we
> can finally register the XXX -> DSI bridge. If they're not (because
> some devs connected to the DSI bus have not been probed yet), then we
> do not register the drm_bridge and wait for the next dsi_host->attach()
> event.
>
> With this solution, I think we can avoid the chicken&egg problem where
> the adv7533 dev is waiting for the DSI host to be probed to be able to
> register a DSI dev with mipi_dsi_device_register_full() and the DSI
> host needs all devs to be registered to not return -EPROBE_DEFER.

I've now tried having mipi_dsi_device_register_full() succeed early and
just do the device-add part when the host shows up.  The problem is
there's still mipi_dsi_attach(), which needs to be delayed until the
panel driver fills in the rest of mipi_dsi_device's fields.  Why aren't
those part of the info?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/24/2017 03:20 AM, Eric Anholt wrote:
> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
>> On Thu, 22 Jun 2017 13:47:43 +0530
>> Archit Taneja <architt@codeaurora.org> wrote:
>>
>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>   
>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>>>   
>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>   
>>>>>>>>
>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>> panel-bridge driver as a module?
>>>>>>>>
>>>>>>>>
>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>> conditions if the userspace decides to use them immediately.
>>>>>>>
>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>>>
>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>> related modules are inserted, and only then proceed to create a drm device.
>>>>>
>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>> bridge attach step, but drm_panel doesn't have an attach step.
>>>
>>> I'm not sure how we can get around this. We had discussion about this on irc
>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>> the real issue.
>>
>> How about making DSI dev registration fully asynchronous, that is, DSI
>> devs declared in the DT under the DSI host node will be
>> registered/attached at probe time, and devs using another control bus
>> (like the adv7533 controller over i2c) will be registered afterwards.
>>
>> That implies moving the drm_brige registration logic outside of the DSI
>> host ->probe() path. The idea would be to check if all devs connected
>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>> can finally register the XXX -> DSI bridge. If they're not (because
>> some devs connected to the DSI bus have not been probed yet), then we
>> do not register the drm_bridge and wait for the next dsi_host->attach()
>> event.
>>
>> With this solution, I think we can avoid the chicken&egg problem where
>> the adv7533 dev is waiting for the DSI host to be probed to be able to
>> register a DSI dev with mipi_dsi_device_register_full() and the DSI
>> host needs all devs to be registered to not return -EPROBE_DEFER.
> 
> I've now tried having mipi_dsi_device_register_full() succeed early and
> just do the device-add part when the host shows up.  The problem is
> there's still mipi_dsi_attach(), which needs to be delayed until the
> panel driver fills in the rest of mipi_dsi_device's fields.  Why aren't
> those part of the info?

In the past, the only way to create mipi_dsi_devices was to add them as
children DT nodes under the DSI host node. The KMS driver calls
mipi_dsi_host_register(), which resulted in the creation of all the
children mipi_dsi_devices. The DSI host was still unaware of parameters
like number of lanes, mode_flags etc. mipi_dsi_attach() was a way by
which the DSI device could share these with its DSI host. Another use
of mipi_dsi_attach()/detach() is if the panel driver wants to notify
the DSI host an update in one of the device's fields (for example, a
change in the number of lanes).

We've recently added another way of creating mipi_dsi_devices (i.e, by
making a driver call mipi_dsi_device_register_full) for cases where there
isn't a DT representation of the DSI device.

For the second method, the need to call mipi_dsi_attach() separately becomes
sort of redundant. We could consider embedding the other mipi_dsi_device
fields (lanes etc) in mipi_dsi_device_info itself, and invoke the DSI host's
->attach() callback through mipi_dsi_device_register_full() itself. Would
that help resolving your issue?

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Archit Taneja]

On 06/24/2017 03:20 AM, Eric Anholt wrote:
> Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> writes:
> 
>> On Thu, 22 Jun 2017 13:47:43 +0530
>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>
>>> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>>>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>:
>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>   
>>>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>>>> Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>>>>>>>   
>>>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>>>> the connector, so that userspace can get at it.
>>>>>>>>>   
>>>>>>>>
>>>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>>>> panel-bridge driver as a module?
>>>>>>>>
>>>>>>>>
>>>>>>>> All the connectors that have been added are registered automatically
>>>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>>>> conditions if the userspace decides to use them immediately.
>>>>>>>
>>>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>>>> which in the case of a panel module that creates the DSI device
>>>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>>>
>>>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>>>> related modules are inserted, and only then proceed to create a drm device.
>>>>>
>>>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>>>> The adv7533 driver gets around this by registering the DSI device in the
>>>>> bridge attach step, but drm_panel doesn't have an attach step.
>>>
>>> I'm not sure how we can get around this. We had discussion about this on irc
>>> recently, but couldn't come up with a good conclusion. We could come up with a
>>> panel_attach() callback to make it similar to bridges, but that's just us avoiding
>>> the real issue.
>>
>> How about making DSI dev registration fully asynchronous, that is, DSI
>> devs declared in the DT under the DSI host node will be
>> registered/attached at probe time, and devs using another control bus
>> (like the adv7533 controller over i2c) will be registered afterwards.
>>
>> That implies moving the drm_brige registration logic outside of the DSI
>> host ->probe() path. The idea would be to check if all devs connected
>> to the DSI bus are ready at dsi_host->attach() time. If they are, we
>> can finally register the XXX -> DSI bridge. If they're not (because
>> some devs connected to the DSI bus have not been probed yet), then we
>> do not register the drm_bridge and wait for the next dsi_host->attach()
>> event.
>>
>> With this solution, I think we can avoid the chicken&egg problem where
>> the adv7533 dev is waiting for the DSI host to be probed to be able to
>> register a DSI dev with mipi_dsi_device_register_full() and the DSI
>> host needs all devs to be registered to not return -EPROBE_DEFER.
> 
> I've now tried having mipi_dsi_device_register_full() succeed early and
> just do the device-add part when the host shows up.  The problem is
> there's still mipi_dsi_attach(), which needs to be delayed until the
> panel driver fills in the rest of mipi_dsi_device's fields.  Why aren't
> those part of the info?

In the past, the only way to create mipi_dsi_devices was to add them as
children DT nodes under the DSI host node. The KMS driver calls
mipi_dsi_host_register(), which resulted in the creation of all the
children mipi_dsi_devices. The DSI host was still unaware of parameters
like number of lanes, mode_flags etc. mipi_dsi_attach() was a way by
which the DSI device could share these with its DSI host. Another use
of mipi_dsi_attach()/detach() is if the panel driver wants to notify
the DSI host an update in one of the device's fields (for example, a
change in the number of lanes).

We've recently added another way of creating mipi_dsi_devices (i.e, by
making a driver call mipi_dsi_device_register_full) for cases where there
isn't a DT representation of the DSI device.

For the second method, the need to call mipi_dsi_attach() separately becomes
sort of redundant. We could consider embedding the other mipi_dsi_device
fields (lanes etc) in mipi_dsi_device_info itself, and invoke the DSI host's
->attach() callback through mipi_dsi_device_register_full() itself. Would
that help resolving your issue?

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Philippe CORNU]

On 06/22/2017 10:17 AM, Archit Taneja wrote:
> 
> 
> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>>> Archit Taneja <architt@codeaurora.org> writes:
>>>
>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>
>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>> the connector, so that userspace can get at it.
>>>>>>>
>>>>>>
>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>> panel-bridge driver as a module?
>>>>>>
>>>>>>
>>>>>> All the connectors that have been added are registered automatically
>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>> conditions if the userspace decides to use them immediately.
>>>>>
>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>> which in the case of a panel module that creates the DSI device
>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>
>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>> related modules are inserted, and only then proceed to create a drm device.
>>>
>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>> The adv7533 driver gets around this by registering the DSI device in the
>>> bridge attach step, but drm_panel doesn't have an attach step.
> 
> I'm not sure how we can get around this. We had discussion about this on irc
> recently, but couldn't come up with a good conclusion. We could come up with a
> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> the real issue.
> 
>>>
>>> Another alternative is my original version of the panel driver that was
>>> a mipi_dsi_device driver that registered the panel during the DSI device
>>> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
>>> phase, currently.
>>
> 
> This would require you to have a DSI device node in DT, rather than an i2c
> node, right? I don't know if we should do that because of a limitation in
> our drm_mipi_dsi and drm_panel frameworks.
> 
> Does anyone have better ideas?
> 
> Thanks,
> Archit
> 
>> + Philippe in copy because we have the same probing issue when adding
>> panel-bridge
>> with the dsi bridge
>>

When adding panel-bridge support to the Synopsys DesignWare DSI bridge, 
I came to the conclusion that the only solution to make it works 
properly (without patching drm) was to "add the DSI bridge" at the end 
of the dw_mipi_dsi_host_attach() (see 
https://lists.freedesktop.org/archives/dri-devel/2017-June/144717.html) 
ie. defers crtc & encoder probing until the panel-bridge connector is 
created.

Before that, I spent a lot of time trying different solutions like 
patching panel-bridge as Eric did (drm_connector_register...), adding 
bind/unbind() everywhere, debugging around __drm_mode_object_add...
Surely DSI Host & device mechanism + panels add complexity to the 
related connector creation...

After reading this thread, I have no good solution to suggest... and 
"deferring probing until connector registration" works fine now on my 
side and seems to be the way others drivers work...

Philippe

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Philippe CORNU]

On 06/22/2017 10:17 AM, Archit Taneja wrote:
> 
> 
> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>>> Archit Taneja <architt@codeaurora.org> writes:
>>>
>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>
>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>> the connector, so that userspace can get at it.
>>>>>>>
>>>>>>
>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>> panel-bridge driver as a module?
>>>>>>
>>>>>>
>>>>>> All the connectors that have been added are registered automatically
>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>> conditions if the userspace decides to use them immediately.
>>>>>
>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>> which in the case of a panel module that creates the DSI device
>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>
>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>> related modules are inserted, and only then proceed to create a drm device.
>>>
>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>> The adv7533 driver gets around this by registering the DSI device in the
>>> bridge attach step, but drm_panel doesn't have an attach step.
> 
> I'm not sure how we can get around this. We had discussion about this on irc
> recently, but couldn't come up with a good conclusion. We could come up with a
> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> the real issue.
> 
>>>
>>> Another alternative is my original version of the panel driver that was
>>> a mipi_dsi_device driver that registered the panel during the DSI device
>>> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
>>> phase, currently.
>>
> 
> This would require you to have a DSI device node in DT, rather than an i2c
> node, right? I don't know if we should do that because of a limitation in
> our drm_mipi_dsi and drm_panel frameworks.
> 
> Does anyone have better ideas?
> 
> Thanks,
> Archit
> 
>> + Philippe in copy because we have the same probing issue when adding
>> panel-bridge
>> with the dsi bridge
>>

When adding panel-bridge support to the Synopsys DesignWare DSI bridge, 
I came to the conclusion that the only solution to make it works 
properly (without patching drm) was to "add the DSI bridge" at the end 
of the dw_mipi_dsi_host_attach() (see 
https://lists.freedesktop.org/archives/dri-devel/2017-June/144717.html) 
ie. defers crtc & encoder probing until the panel-bridge connector is 
created.

Before that, I spent a lot of time trying different solutions like 
patching panel-bridge as Eric did (drm_connector_register...), adding 
bind/unbind() everywhere, debugging around __drm_mode_object_add...
Surely DSI Host & device mechanism + panels add complexity to the 
related connector creation...

After reading this thread, I have no good solution to suggest... and 
"deferring probing until connector registration" works fine now on my 
side and seems to be the way others drivers work...

Philippe

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

Archit Taneja <architt@codeaurora.org> writes:

> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>>> Archit Taneja <architt@codeaurora.org> writes:
>>>
>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>
>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>> the connector, so that userspace can get at it.
>>>>>>>
>>>>>>
>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>> panel-bridge driver as a module?
>>>>>>
>>>>>>
>>>>>> All the connectors that have been added are registered automatically
>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>> conditions if the userspace decides to use them immediately.
>>>>>
>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>> which in the case of a panel module that creates the DSI device
>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>
>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>> related modules are inserted, and only then proceed to create a drm device.
>>>
>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>> The adv7533 driver gets around this by registering the DSI device in the
>>> bridge attach step, but drm_panel doesn't have an attach step.
>
> I'm not sure how we can get around this. We had discussion about this on irc
> recently, but couldn't come up with a good conclusion. We could come up with a
> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> the real issue.
>
>>>
>>> Another alternative is my original version of the panel driver that was
>>> a mipi_dsi_device driver that registered the panel during the DSI device
>>> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
>>> phase, currently.
>> 
>
> This would require you to have a DSI device node in DT, rather than an i2c
> node, right? I don't know if we should do that because of a limitation in
> our drm_mipi_dsi and drm_panel frameworks.

All versions of this patch have had one of those, because either that's
where you attach the driver (the first, no-core-changes version of the
driver) or you need it for the of-graph connections anyway.  These later
versions just haven't had a compatible string on the DSI device node.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Eric Anholt]

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

Archit Taneja <architt@codeaurora.org> writes:

> On 06/22/2017 01:20 PM, Benjamin Gaignard wrote:
>> 2017-06-20 19:31 GMT+02:00 Eric Anholt <eric@anholt.net>:
>>> Archit Taneja <architt@codeaurora.org> writes:
>>>
>>>> On 06/16/2017 08:13 PM, Eric Anholt wrote:
>>>>> Archit Taneja <architt@codeaurora.org> writes:
>>>>>
>>>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote:
>>>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(),
>>>>>>> then the connector's state would never get initialized, and we'd
>>>>>>> dereference the NULL in the hotplug path.  We also need to register
>>>>>>> the connector, so that userspace can get at it.
>>>>>>>
>>>>>>
>>>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before
>>>>>> drm_mode_config_reset? Is it the case when we're inserting the
>>>>>> panel-bridge driver as a module?
>>>>>>
>>>>>>
>>>>>> All the connectors that have been added are registered automatically
>>>>>> when drm_dev_register() is called by the KMS driver. Registering a
>>>>>> connector in the middle of setting up our driver is prone to race
>>>>>> conditions if the userspace decides to use them immediately.
>>>>>
>>>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time,
>>>>> which in the case of a panel module that creates the DSI device
>>>>> (adv7533-style, like you said I should use as a reference) will be after
>>>>> drm_mode_config_reset() and drm_dev_register().
>>>>
>>>> Okay. In the case of the msm kms driver, we defer probe until the
>>>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset()
>>>> and drm_dev_register(). I assumed this was the general practice followed by
>>>> most kms drivers. I.,e the kms driver defers probe until all connector
>>>> related modules are inserted, and only then proceed to create a drm device.
>>>
>>> The problem, though, is the panel driver needs the MIPI DSI host to
>>> exist to call mipi_dsi_device_register_full() during the probe process.
>>> The adv7533 driver gets around this by registering the DSI device in the
>>> bridge attach step, but drm_panel doesn't have an attach step.
>
> I'm not sure how we can get around this. We had discussion about this on irc
> recently, but couldn't come up with a good conclusion. We could come up with a
> panel_attach() callback to make it similar to bridges, but that's just us avoiding
> the real issue.
>
>>>
>>> Another alternative is my original version of the panel driver that was
>>> a mipi_dsi_device driver that registered the panel during the DSI device
>>> probe.  That's why vc4's panel lookup is during the MIPI DSI attach
>>> phase, currently.
>> 
>
> This would require you to have a DSI device node in DT, rather than an i2c
> node, right? I don't know if we should do that because of a limitation in
> our drm_mipi_dsi and drm_panel frameworks.

All versions of this patch have had one of those, because either that's
where you attach the driver (the first, no-core-changes version of the
driver) or you need it for the of-graph connections anyway.  These later
versions just haven't had a compatible string on the DSI device node.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Daniel Vetter]

On Thu, Jun 15, 2017 at 01:41:24PM -0700, Eric Anholt wrote:
> If the panel-bridge is being set up after the drm_mode_config_reset(),
> then the connector's state would never get initialized, and we'd
> dereference the NULL in the hotplug path.  We also need to register
> the connector, so that userspace can get at it.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

I've not read all the details of the discussion here, just a comment on
what this implies if we go with this.

> ---
>  drivers/gpu/drm/bridge/panel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 67fe19e5a9c6..8ed8a70799c7 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge)
>  
>  	drm_mode_connector_attach_encoder(&panel_bridge->connector,
>  					  bridge->encoder);
> +	drm_atomic_helper_connector_reset(&panel_bridge->connector);

I'm not sure we want to tie the ->reset stuff in with other helpers. One
of the design goals I had with all things atomic was to make helpers much
more modular, and imo the reset is one such piece - a driver might want to
instead reconstruct state from hw using something like i915's hw state
readout/compare/verify framework.

If we can solve this init ordering issue without adding a depency on the
reset stuff that would be a bit neater imo. But that's just my 2 cents.

Cheers, Daniel

>  
>  	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
>  	if (ret < 0)
>  		return ret;
>  
> +	drm_connector_register(&panel_bridge->connector);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge.

[Daniel Vetter]

On Thu, Jun 15, 2017 at 01:41:24PM -0700, Eric Anholt wrote:
> If the panel-bridge is being set up after the drm_mode_config_reset(),
> then the connector's state would never get initialized, and we'd
> dereference the NULL in the hotplug path.  We also need to register
> the connector, so that userspace can get at it.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>

I've not read all the details of the discussion here, just a comment on
what this implies if we go with this.

> ---
>  drivers/gpu/drm/bridge/panel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 67fe19e5a9c6..8ed8a70799c7 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -82,11 +82,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge)
>  
>  	drm_mode_connector_attach_encoder(&panel_bridge->connector,
>  					  bridge->encoder);
> +	drm_atomic_helper_connector_reset(&panel_bridge->connector);

I'm not sure we want to tie the ->reset stuff in with other helpers. One
of the design goals I had with all things atomic was to make helpers much
more modular, and imo the reset is one such piece - a driver might want to
instead reconstruct state from hw using something like i915's hw state
readout/compare/verify framework.

If we can solve this init ordering issue without adding a depency on the
reset stuff that would be a bit neater imo. But that's just my 2 cents.

Cheers, Daniel

>  
>  	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
>  	if (ret < 0)
>  		return ret;
>  
> +	drm_connector_register(&panel_bridge->connector);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm/vc4: Fix DSI T_INIT timing.

[Eric Anholt]

The DPHY spec requires a much larger T_INIT than I was specifying
before.  In the absence of clear specs from the slave of what their
timing is, just use the value that the firmware was using.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 5e8b81eaa168..15f6d5005ab9 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1035,7 +1035,17 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 				     DSI_HS_DLT4_TRAIL) |
 		       VC4_SET_FIELD(0, DSI_HS_DLT4_ANLAT));
 
-	DSI_PORT_WRITE(HS_DLT5, VC4_SET_FIELD(dsi_hs_timing(ui_ns, 1000, 5000),
+	/* T_INIT is how long STOP is driven after power-up to
+	 * indicate to the slave (also coming out of power-up) that
+	 * master init is complete, and should be greater than the
+	 * maximum of two value: T_INIT,MASTER and T_INIT,SLAVE.  The
+	 * D-PHY spec gives a minimum 100us for T_INIT,MASTER and
+	 * T_INIT,SLAVE, while allowing protocols on top of it to give
+	 * greater minimums.  The vc4 firmware uses an extremely
+	 * conservative 5ms, and we maintain that here.
+	 */
+	DSI_PORT_WRITE(HS_DLT5, VC4_SET_FIELD(dsi_hs_timing(ui_ns,
+							    5 * 1000 * 1000, 0),
 					      DSI_HS_DLT5_INIT));
 
 	DSI_PORT_WRITE(HS_DLT6,
-- 
2.11.0

[PATCH 2/7] drm/vc4: Fix DSI T_INIT timing.

[Eric Anholt]

The DPHY spec requires a much larger T_INIT than I was specifying
before.  In the absence of clear specs from the slave of what their
timing is, just use the value that the firmware was using.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 5e8b81eaa168..15f6d5005ab9 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1035,7 +1035,17 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 				     DSI_HS_DLT4_TRAIL) |
 		       VC4_SET_FIELD(0, DSI_HS_DLT4_ANLAT));
 
-	DSI_PORT_WRITE(HS_DLT5, VC4_SET_FIELD(dsi_hs_timing(ui_ns, 1000, 5000),
+	/* T_INIT is how long STOP is driven after power-up to
+	 * indicate to the slave (also coming out of power-up) that
+	 * master init is complete, and should be greater than the
+	 * maximum of two value: T_INIT,MASTER and T_INIT,SLAVE.  The
+	 * D-PHY spec gives a minimum 100us for T_INIT,MASTER and
+	 * T_INIT,SLAVE, while allowing protocols on top of it to give
+	 * greater minimums.  The vc4 firmware uses an extremely
+	 * conservative 5ms, and we maintain that here.
+	 */
+	DSI_PORT_WRITE(HS_DLT5, VC4_SET_FIELD(dsi_hs_timing(ui_ns,
+							    5 * 1000 * 1000, 0),
 					      DSI_HS_DLT5_INIT));
 
 	DSI_PORT_WRITE(HS_DLT6,
-- 
2.11.0

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

[PATCH 3/7] drm/vc4: Fix misleading name of the continuous flag.

[Eric Anholt]

The logic was all right in the end, the name was just backwards.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 15f6d5005ab9..629d372633e6 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -736,18 +736,18 @@ static void vc4_dsi_latch_ulps(struct vc4_dsi *dsi, bool latch)
 /* Enters or exits Ultra Low Power State. */
 static void vc4_dsi_ulps(struct vc4_dsi *dsi, bool ulps)
 {
-	bool continuous = dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS;
-	u32 phyc_ulps = ((continuous ? DSI_PORT_BIT(PHYC_CLANE_ULPS) : 0) |
+	bool non_continuous = dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS;
+	u32 phyc_ulps = ((non_continuous ? DSI_PORT_BIT(PHYC_CLANE_ULPS) : 0) |
 			 DSI_PHYC_DLANE0_ULPS |
 			 (dsi->lanes > 1 ? DSI_PHYC_DLANE1_ULPS : 0) |
 			 (dsi->lanes > 2 ? DSI_PHYC_DLANE2_ULPS : 0) |
 			 (dsi->lanes > 3 ? DSI_PHYC_DLANE3_ULPS : 0));
-	u32 stat_ulps = ((continuous ? DSI1_STAT_PHY_CLOCK_ULPS : 0) |
+	u32 stat_ulps = ((non_continuous ? DSI1_STAT_PHY_CLOCK_ULPS : 0) |
 			 DSI1_STAT_PHY_D0_ULPS |
 			 (dsi->lanes > 1 ? DSI1_STAT_PHY_D1_ULPS : 0) |
 			 (dsi->lanes > 2 ? DSI1_STAT_PHY_D2_ULPS : 0) |
 			 (dsi->lanes > 3 ? DSI1_STAT_PHY_D3_ULPS : 0));
-	u32 stat_stop = ((continuous ? DSI1_STAT_PHY_CLOCK_STOP : 0) |
+	u32 stat_stop = ((non_continuous ? DSI1_STAT_PHY_CLOCK_STOP : 0) |
 			 DSI1_STAT_PHY_D0_STOP |
 			 (dsi->lanes > 1 ? DSI1_STAT_PHY_D1_STOP : 0) |
 			 (dsi->lanes > 2 ? DSI1_STAT_PHY_D2_STOP : 0) |
-- 
2.11.0

[PATCH 3/7] drm/vc4: Fix misleading name of the continuous flag.

[Eric Anholt]

The logic was all right in the end, the name was just backwards.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 15f6d5005ab9..629d372633e6 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -736,18 +736,18 @@ static void vc4_dsi_latch_ulps(struct vc4_dsi *dsi, bool latch)
 /* Enters or exits Ultra Low Power State. */
 static void vc4_dsi_ulps(struct vc4_dsi *dsi, bool ulps)
 {
-	bool continuous = dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS;
-	u32 phyc_ulps = ((continuous ? DSI_PORT_BIT(PHYC_CLANE_ULPS) : 0) |
+	bool non_continuous = dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS;
+	u32 phyc_ulps = ((non_continuous ? DSI_PORT_BIT(PHYC_CLANE_ULPS) : 0) |
 			 DSI_PHYC_DLANE0_ULPS |
 			 (dsi->lanes > 1 ? DSI_PHYC_DLANE1_ULPS : 0) |
 			 (dsi->lanes > 2 ? DSI_PHYC_DLANE2_ULPS : 0) |
 			 (dsi->lanes > 3 ? DSI_PHYC_DLANE3_ULPS : 0));
-	u32 stat_ulps = ((continuous ? DSI1_STAT_PHY_CLOCK_ULPS : 0) |
+	u32 stat_ulps = ((non_continuous ? DSI1_STAT_PHY_CLOCK_ULPS : 0) |
 			 DSI1_STAT_PHY_D0_ULPS |
 			 (dsi->lanes > 1 ? DSI1_STAT_PHY_D1_ULPS : 0) |
 			 (dsi->lanes > 2 ? DSI1_STAT_PHY_D2_ULPS : 0) |
 			 (dsi->lanes > 3 ? DSI1_STAT_PHY_D3_ULPS : 0));
-	u32 stat_stop = ((continuous ? DSI1_STAT_PHY_CLOCK_STOP : 0) |
+	u32 stat_stop = ((non_continuous ? DSI1_STAT_PHY_CLOCK_STOP : 0) |
 			 DSI1_STAT_PHY_D0_STOP |
 			 (dsi->lanes > 1 ? DSI1_STAT_PHY_D1_STOP : 0) |
 			 (dsi->lanes > 2 ? DSI1_STAT_PHY_D2_STOP : 0) |
-- 
2.11.0

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

[PATCH 4/7] drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.

[Eric Anholt]

I'm not sure what changed where I started getting vrefresh=0 from the
mode to be fixed up.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 629d372633e6..fca4d7fd677e 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -866,7 +866,9 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
 	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
 
 	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
-	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
+	adjusted_mode->htotal = pixel_clock_hz / (drm_mode_vrefresh(mode) *
+						  mode->vtotal);
+
 	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
 	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
 
-- 
2.11.0

[PATCH 4/7] drm/vc4: Use drm_mode_vrefresh() in DSI fixup, in case vrefresh is 0.

[Eric Anholt]

I'm not sure what changed where I started getting vrefresh=0 from the
mode to be fixed up.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 629d372633e6..fca4d7fd677e 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -866,7 +866,9 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
 	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
 
 	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
-	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
+	adjusted_mode->htotal = pixel_clock_hz / (drm_mode_vrefresh(mode) *
+						  mode->vtotal);
+
 	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
 	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
 
-- 
2.11.0

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

[PATCH 5/7] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

[Eric Anholt]

This doesn't yet cover input, but the driver does get the display
working when the firmware is disabled from talking to our I2C lines.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../panel/raspberrypi,7inch-touchscreen.txt        | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt

diff --git a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
new file mode 100644
index 000000000000..e9e19c059260
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
@@ -0,0 +1,49 @@
+This binding covers the official 7" (800x480) Raspberry Pi touchscreen
+panel.
+
+This DSI panel contains:
+
+- TC358762 DSI->DPI bridge
+- Atmel microcontroller on I2C for power sequencing the DSI bridge and
+  controlling backlight
+- Touchscreen controller on I2C for touch input
+
+and this binding covers the DSI display parts but not its touch input.
+
+Required properties:
+- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"
+- reg:		Must be "45"
+- port:		See panel-common.txt
+
+Example:
+
+dsi1: dsi@7e700000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	<...>
+
+	port {
+		dsi_out_port: endpoint {
+			remote-endpoint = <&panel_dsi_port>;
+		};
+	};
+};
+
+i2c_dsi: i2c {
+	compatible = "i2c-gpio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+
+	lcd@45 {
+		compatible = "raspberrypi,7inch-touchscreen-panel";
+		reg = <0x45>;
+
+		port {
+			panel_dsi_port: endpoint {
+				remote-endpoint = <&dsi_out_port>;
+			};
+		};
+	};
+};
-- 
2.11.0

Re: [PATCH 5/7] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

[Rob Herring]

On Thu, Jun 15, 2017 at 01:41:28PM -0700, Eric Anholt wrote:
> This doesn't yet cover input, but the driver does get the display
> working when the firmware is disabled from talking to our I2C lines.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  .../panel/raspberrypi,7inch-touchscreen.txt        | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 5/7] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

[Rob Herring]

On Thu, Jun 15, 2017 at 01:41:28PM -0700, Eric Anholt wrote:
> This doesn't yet cover input, but the driver does get the display
> working when the firmware is disabled from talking to our I2C lines.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  .../panel/raspberrypi,7inch-touchscreen.txt        | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt

Acked-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/7] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

[Eric Anholt]

This driver communicates with the Atmel microcontroller for sequencing
the poweron of the TC358762 DSI-DPI bridge and controlling the
backlight PWM.

v2: Set the same default orientation as the closed source firmware
    used, which is the best for viewing angle.
v3: Rewrite as an i2c client driver after bridge driver rejection.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 507 +++++++++++++++++++++
 3 files changed, 516 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 3e29a9903303..4111f497945d 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -62,6 +62,14 @@ config DRM_PANEL_PANASONIC_VVX10F034N00
 	  WUXGA (1920x1200) Novatek NT1397-based DSI panel as found in some
 	  Xperia Z2 tablets
 
+config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
+	tristate "Raspberry Pi 7-inch touchscreen panel"
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for the Raspberry
+	  Pi 7" Touchscreen.  To compile this driver as a module,
+	  choose M here.
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 292b3c77aede..fbb8a2373058 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
+obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
new file mode 100644
index 000000000000..da398785eac7
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -0,0 +1,507 @@
+/*
+ * Copyright © 2016-2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Portions of this file (derived from panel-simple.c) are:
+ *
+ * Copyright (C) 2013, NVIDIA Corporation.  All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Raspberry Pi 7" touchscreen panel driver.
+ *
+ * The 7" touchscreen consists of a DPI LCD panel, a Toshiba
+ * TC358762XBG DSI-DPI bridge, and an I2C-connected Atmel ATTINY88-MUR
+ * controlling power management, the LCD PWM, and initial register
+ * setup of the Tohsiba.
+ *
+ * This driver controls the TC358762 and ATTINY88, presenting a DSI
+ * device with a drm_panel.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pm.h>
+
+#include <drm/drm_panel.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define RPI_DSI_DRIVER_NAME "rpi-ts-dsi"
+
+/* I2C registers of the Atmel microcontroller. */
+enum REG_ADDR {
+	REG_ID = 0x80,
+	REG_PORTA, /* BIT(2) for horizontal flip, BIT(3) for vertical flip */
+	REG_PORTB,
+	REG_PORTC,
+	REG_PORTD,
+	REG_POWERON,
+	REG_PWM,
+	REG_DDRA,
+	REG_DDRB,
+	REG_DDRC,
+	REG_DDRD,
+	REG_TEST,
+	REG_WR_ADDRL,
+	REG_WR_ADDRH,
+	REG_READH,
+	REG_READL,
+	REG_WRITEH,
+	REG_WRITEL,
+	REG_ID2,
+};
+
+/* We only turn the PWM on or off, without varying values. */
+#define RPI_TOUCHSCREEN_MAX_BRIGHTNESS 1
+
+/* DSI D-PHY Layer Registers */
+#define D0W_DPHYCONTTX		0x0004
+#define CLW_DPHYCONTRX		0x0020
+#define D0W_DPHYCONTRX		0x0024
+#define D1W_DPHYCONTRX		0x0028
+#define COM_DPHYCONTRX		0x0038
+#define CLW_CNTRL		0x0040
+#define D0W_CNTRL		0x0044
+#define D1W_CNTRL		0x0048
+#define DFTMODE_CNTRL		0x0054
+
+/* DSI PPI Layer Registers */
+#define PPI_STARTPPI		0x0104
+#define PPI_BUSYPPI		0x0108
+#define PPI_LINEINITCNT		0x0110
+#define PPI_LPTXTIMECNT		0x0114
+#define PPI_CLS_ATMR		0x0140
+#define PPI_D0S_ATMR		0x0144
+#define PPI_D1S_ATMR		0x0148
+#define PPI_D0S_CLRSIPOCOUNT	0x0164
+#define PPI_D1S_CLRSIPOCOUNT	0x0168
+#define CLS_PRE			0x0180
+#define D0S_PRE			0x0184
+#define D1S_PRE			0x0188
+#define CLS_PREP		0x01A0
+#define D0S_PREP		0x01A4
+#define D1S_PREP		0x01A8
+#define CLS_ZERO		0x01C0
+#define D0S_ZERO		0x01C4
+#define D1S_ZERO		0x01C8
+#define PPI_CLRFLG		0x01E0
+#define PPI_CLRSIPO		0x01E4
+#define HSTIMEOUT		0x01F0
+#define HSTIMEOUTENABLE		0x01F4
+
+/* DSI Protocol Layer Registers */
+#define DSI_STARTDSI		0x0204
+#define DSI_BUSYDSI		0x0208
+#define DSI_LANEENABLE		0x0210
+# define DSI_LANEENABLE_CLOCK		BIT(0)
+# define DSI_LANEENABLE_D0		BIT(1)
+# define DSI_LANEENABLE_D1		BIT(2)
+
+#define DSI_LANESTATUS0		0x0214
+#define DSI_LANESTATUS1		0x0218
+#define DSI_INTSTATUS		0x0220
+#define DSI_INTMASK		0x0224
+#define DSI_INTCLR		0x0228
+#define DSI_LPTXTO		0x0230
+#define DSI_MODE		0x0260
+#define DSI_PAYLOAD0		0x0268
+#define DSI_PAYLOAD1		0x026C
+#define DSI_SHORTPKTDAT		0x0270
+#define DSI_SHORTPKTREQ		0x0274
+#define DSI_BTASTA		0x0278
+#define DSI_BTACLR		0x027C
+
+/* DSI General Registers */
+#define DSIERRCNT		0x0300
+#define DSISIGMOD		0x0304
+
+/* DSI Application Layer Registers */
+#define APLCTRL			0x0400
+#define APLSTAT			0x0404
+#define APLERR			0x0408
+#define PWRMOD			0x040C
+#define RDPKTLN			0x0410
+#define PXLFMT			0x0414
+#define MEMWRCMD		0x0418
+
+/* LCDC/DPI Host Registers */
+#define LCDCTRL			0x0420
+#define HSR			0x0424
+#define HDISPR			0x0428
+#define VSR			0x042C
+#define VDISPR			0x0430
+#define VFUEN			0x0434
+
+/* DBI-B Host Registers */
+#define DBIBCTRL		0x0440
+
+/* SPI Master Registers */
+#define SPICMR			0x0450
+#define SPITCR			0x0454
+
+/* System Controller Registers */
+#define SYSSTAT			0x0460
+#define SYSCTRL			0x0464
+#define SYSPLL1			0x0468
+#define SYSPLL2			0x046C
+#define SYSPLL3			0x0470
+#define SYSPMCTRL		0x047C
+
+/* GPIO Registers */
+#define GPIOC			0x0480
+#define GPIOO			0x0484
+#define GPIOI			0x0488
+
+/* I2C Registers */
+#define I2CCLKCTRL		0x0490
+
+/* Chip/Rev Registers */
+#define IDREG			0x04A0
+
+/* Debug Registers */
+#define WCMDQUEUE		0x0500
+#define RCMDQUEUE		0x0504
+
+struct rpi_touchscreen {
+	struct drm_panel base;
+	struct mipi_dsi_device *dsi;
+	struct i2c_client *i2c;
+};
+
+static const struct drm_display_mode rpi_touchscreen_modes[] = {
+	{
+		/* Modeline comes from the Raspberry Pi firmware, with HFP=1
+		 * plugged in and clock re-computed from that.
+		 */
+		.clock = 25979400 / 1000,
+		.hdisplay = 800,
+		.hsync_start = 800 + 1,
+		.hsync_end = 800 + 1 + 2,
+		.htotal = 800 + 1 + 2 + 46,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 2,
+		.vtotal = 480 + 7 + 2 + 21,
+		.vrefresh = 60,
+	},
+};
+
+static struct rpi_touchscreen *panel_to_ts(struct drm_panel *panel)
+{
+	return container_of(panel, struct rpi_touchscreen, base);
+}
+
+static u8 rpi_touchscreen_i2c_read(struct rpi_touchscreen *ts, u8 reg)
+{
+	return i2c_smbus_read_byte_data(ts->i2c, reg);
+}
+
+static void rpi_touchscreen_i2c_write(struct rpi_touchscreen *ts,
+				      u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
+	if (ret)
+		dev_err(&ts->dsi->dev, "I2C write failed: %d\n", ret);
+}
+
+static int rpi_touchscreen_write(struct rpi_touchscreen *ts, u16 reg, u32 val)
+{
+#if 0
+	/* The firmware uses LP DSI transactions like this to bring up
+	 * the hardware, which should be faster than using I2C to then
+	 * pass to the Toshiba.  However, I was unable to get it to
+	 * work.
+	 */
+	u8 msg[] = {
+		reg,
+		reg >> 8,
+		val,
+		val >> 8,
+		val >> 16,
+		val >> 24,
+	};
+
+	mipi_dsi_dcs_write_buffer(ts->dsi, msg, sizeof(msg));
+#else
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRH, reg >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRL, reg);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEH, val >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEL, val);
+#endif
+
+	return 0;
+}
+
+static int rpi_touchscreen_disable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+	udelay(1);
+
+	return 0;
+}
+
+static int rpi_touchscreen_noop(struct drm_panel *panel)
+{
+	return 0;
+}
+
+static int rpi_touchscreen_enable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+	int i;
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
+	/* Wait for nPWRDWN to go low to indicate poweron is done. */
+	for (i = 0; i < 100; i++) {
+		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
+			break;
+	}
+
+	rpi_touchscreen_write(ts, DSI_LANEENABLE,
+			      DSI_LANEENABLE_CLOCK |
+			      DSI_LANEENABLE_D0);
+	rpi_touchscreen_write(ts, PPI_D0S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D1S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D0S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_D1S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_LPTXTIMECNT, 0x03);
+
+	rpi_touchscreen_write(ts, SPICMR, 0x00);
+	rpi_touchscreen_write(ts, LCDCTRL, 0x00100150);
+	rpi_touchscreen_write(ts, SYSCTRL, 0x040f);
+	msleep(100);
+
+	rpi_touchscreen_write(ts, PPI_STARTPPI, 0x01);
+	rpi_touchscreen_write(ts, DSI_STARTDSI, 0x01);
+	msleep(100);
+
+	/* Turn on the backlight. */
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+
+	/* Default to the same orientation as the closed source
+	 * firmware used for the panel.  Runtime rotation
+	 * configuration will be supported using VC4's plane
+	 * orientation bits.
+	 */
+	rpi_touchscreen_i2c_write(ts, REG_PORTA, BIT(2));
+
+	return 0;
+}
+
+static int rpi_touchscreen_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct drm_device *drm = panel->drm;
+	unsigned int i, num = 0;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_touchscreen_modes); i++) {
+		const struct drm_display_mode *m = &rpi_touchscreen_modes[i];
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_duplicate(drm, m);
+		if (!mode) {
+			dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, m->vrefresh);
+			continue;
+		}
+
+		mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (i == 0)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+
+		drm_mode_probed_add(connector, mode);
+		num++;
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = 154;
+	connector->display_info.height_mm = 86;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return num;
+}
+
+static const struct drm_panel_funcs rpi_touchscreen_funcs = {
+	.disable = rpi_touchscreen_disable,
+	.unprepare = rpi_touchscreen_noop,
+	.prepare = rpi_touchscreen_noop,
+	.enable = rpi_touchscreen_enable,
+	.get_modes = rpi_touchscreen_get_modes,
+};
+
+static int rpi_touchscreen_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct rpi_touchscreen *ts;
+	struct device_node *dsi_host_node, *endpoint;
+	struct mipi_dsi_host *host;
+	int ret, ver;
+	struct mipi_dsi_device_info info = {
+		.type = RPI_DSI_DRIVER_NAME,
+		.channel = 0,
+		.node = NULL,
+	};
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, ts);
+
+	ts->i2c = i2c;
+
+	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
+	if (ver < 0) {
+		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
+		return -ENODEV;
+	}
+
+	switch (ver) {
+	case 0xde: /* ver 1 */
+	case 0xc3: /* ver 2 */
+		break;
+	default:
+		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
+		return -ENODEV;
+	}
+
+	/* Turn off at boot, so we can cleanly sequence powering on. */
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	dsi_host_node = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+
+	host = of_find_mipi_dsi_host_by_node(dsi_host_node);
+	of_node_put(dsi_host_node);
+
+	if (!host) {
+		dev_dbg(dev, "MIPI DSI host not registered yet\n");
+		return -EPROBE_DEFER;
+	}
+
+	ts->dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(ts->dsi)) {
+		dev_err(dev, "DSI device registration failed: %ld\n",
+			PTR_ERR(ts->dsi));
+		return PTR_ERR(ts->dsi);
+	}
+
+	ts->dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
+			       MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			       MIPI_DSI_MODE_LPM);
+	ts->dsi->format = MIPI_DSI_FMT_RGB888;
+	ts->dsi->lanes = 1;
+
+	ts->base.dev = &ts->dsi->dev;
+	ts->base.funcs = &rpi_touchscreen_funcs;
+
+	ret = drm_panel_add(&ts->base);
+	if (ret)
+		return ret;
+
+	ret = mipi_dsi_attach(ts->dsi);
+	if (ret) {
+		dev_err(dev, "failed to attach dsi to host\n");
+		drm_panel_remove(&ts->base);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rpi_touchscreen_remove(struct i2c_client *i2c)
+{
+	struct rpi_touchscreen *ts = i2c_get_clientdata(i2c);
+
+	mipi_dsi_detach(ts->dsi);
+
+	drm_panel_remove(&ts->base);
+
+	mipi_dsi_device_unregister(ts->dsi);
+	kfree(ts->dsi);
+
+	return 0;
+}
+
+static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = {
+	.driver.name = RPI_DSI_DRIVER_NAME,
+};
+
+static const struct of_device_id rpi_touchscreen_of_ids[] = {
+	{ .compatible = "raspberrypi,7inch-touchscreen-panel" },
+	{ } /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, rpi_touchscreen_of_ids);
+
+static struct i2c_driver rpi_touchscreen_driver = {
+	.driver = {
+		.name = "rpi_touchscreen",
+		.of_match_table = rpi_touchscreen_of_ids,
+	},
+	.probe = rpi_touchscreen_probe,
+	.remove = rpi_touchscreen_remove,
+};
+
+static int __init rpi_touchscreen_init(void)
+{
+	mipi_dsi_driver_register(&rpi_touchscreen_dsi_driver);
+	return i2c_add_driver(&rpi_touchscreen_driver);
+}
+module_init(rpi_touchscreen_init);
+
+static void __exit rpi_touchscreen_exit(void)
+{
+	i2c_del_driver(&rpi_touchscreen_driver);
+	mipi_dsi_driver_unregister(&rpi_touchscreen_dsi_driver);
+}
+module_exit(rpi_touchscreen_exit);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi 7-inch touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

[PATCH 6/7] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

[Eric Anholt]

This driver communicates with the Atmel microcontroller for sequencing
the poweron of the TC358762 DSI-DPI bridge and controlling the
backlight PWM.

v2: Set the same default orientation as the closed source firmware
    used, which is the best for viewing angle.
v3: Rewrite as an i2c client driver after bridge driver rejection.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 507 +++++++++++++++++++++
 3 files changed, 516 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 3e29a9903303..4111f497945d 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -62,6 +62,14 @@ config DRM_PANEL_PANASONIC_VVX10F034N00
 	  WUXGA (1920x1200) Novatek NT1397-based DSI panel as found in some
 	  Xperia Z2 tablets
 
+config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
+	tristate "Raspberry Pi 7-inch touchscreen panel"
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for the Raspberry
+	  Pi 7" Touchscreen.  To compile this driver as a module,
+	  choose M here.
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 292b3c77aede..fbb8a2373058 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
+obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
new file mode 100644
index 000000000000..da398785eac7
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -0,0 +1,507 @@
+/*
+ * Copyright © 2016-2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Portions of this file (derived from panel-simple.c) are:
+ *
+ * Copyright (C) 2013, NVIDIA Corporation.  All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Raspberry Pi 7" touchscreen panel driver.
+ *
+ * The 7" touchscreen consists of a DPI LCD panel, a Toshiba
+ * TC358762XBG DSI-DPI bridge, and an I2C-connected Atmel ATTINY88-MUR
+ * controlling power management, the LCD PWM, and initial register
+ * setup of the Tohsiba.
+ *
+ * This driver controls the TC358762 and ATTINY88, presenting a DSI
+ * device with a drm_panel.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pm.h>
+
+#include <drm/drm_panel.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define RPI_DSI_DRIVER_NAME "rpi-ts-dsi"
+
+/* I2C registers of the Atmel microcontroller. */
+enum REG_ADDR {
+	REG_ID = 0x80,
+	REG_PORTA, /* BIT(2) for horizontal flip, BIT(3) for vertical flip */
+	REG_PORTB,
+	REG_PORTC,
+	REG_PORTD,
+	REG_POWERON,
+	REG_PWM,
+	REG_DDRA,
+	REG_DDRB,
+	REG_DDRC,
+	REG_DDRD,
+	REG_TEST,
+	REG_WR_ADDRL,
+	REG_WR_ADDRH,
+	REG_READH,
+	REG_READL,
+	REG_WRITEH,
+	REG_WRITEL,
+	REG_ID2,
+};
+
+/* We only turn the PWM on or off, without varying values. */
+#define RPI_TOUCHSCREEN_MAX_BRIGHTNESS 1
+
+/* DSI D-PHY Layer Registers */
+#define D0W_DPHYCONTTX		0x0004
+#define CLW_DPHYCONTRX		0x0020
+#define D0W_DPHYCONTRX		0x0024
+#define D1W_DPHYCONTRX		0x0028
+#define COM_DPHYCONTRX		0x0038
+#define CLW_CNTRL		0x0040
+#define D0W_CNTRL		0x0044
+#define D1W_CNTRL		0x0048
+#define DFTMODE_CNTRL		0x0054
+
+/* DSI PPI Layer Registers */
+#define PPI_STARTPPI		0x0104
+#define PPI_BUSYPPI		0x0108
+#define PPI_LINEINITCNT		0x0110
+#define PPI_LPTXTIMECNT		0x0114
+#define PPI_CLS_ATMR		0x0140
+#define PPI_D0S_ATMR		0x0144
+#define PPI_D1S_ATMR		0x0148
+#define PPI_D0S_CLRSIPOCOUNT	0x0164
+#define PPI_D1S_CLRSIPOCOUNT	0x0168
+#define CLS_PRE			0x0180
+#define D0S_PRE			0x0184
+#define D1S_PRE			0x0188
+#define CLS_PREP		0x01A0
+#define D0S_PREP		0x01A4
+#define D1S_PREP		0x01A8
+#define CLS_ZERO		0x01C0
+#define D0S_ZERO		0x01C4
+#define D1S_ZERO		0x01C8
+#define PPI_CLRFLG		0x01E0
+#define PPI_CLRSIPO		0x01E4
+#define HSTIMEOUT		0x01F0
+#define HSTIMEOUTENABLE		0x01F4
+
+/* DSI Protocol Layer Registers */
+#define DSI_STARTDSI		0x0204
+#define DSI_BUSYDSI		0x0208
+#define DSI_LANEENABLE		0x0210
+# define DSI_LANEENABLE_CLOCK		BIT(0)
+# define DSI_LANEENABLE_D0		BIT(1)
+# define DSI_LANEENABLE_D1		BIT(2)
+
+#define DSI_LANESTATUS0		0x0214
+#define DSI_LANESTATUS1		0x0218
+#define DSI_INTSTATUS		0x0220
+#define DSI_INTMASK		0x0224
+#define DSI_INTCLR		0x0228
+#define DSI_LPTXTO		0x0230
+#define DSI_MODE		0x0260
+#define DSI_PAYLOAD0		0x0268
+#define DSI_PAYLOAD1		0x026C
+#define DSI_SHORTPKTDAT		0x0270
+#define DSI_SHORTPKTREQ		0x0274
+#define DSI_BTASTA		0x0278
+#define DSI_BTACLR		0x027C
+
+/* DSI General Registers */
+#define DSIERRCNT		0x0300
+#define DSISIGMOD		0x0304
+
+/* DSI Application Layer Registers */
+#define APLCTRL			0x0400
+#define APLSTAT			0x0404
+#define APLERR			0x0408
+#define PWRMOD			0x040C
+#define RDPKTLN			0x0410
+#define PXLFMT			0x0414
+#define MEMWRCMD		0x0418
+
+/* LCDC/DPI Host Registers */
+#define LCDCTRL			0x0420
+#define HSR			0x0424
+#define HDISPR			0x0428
+#define VSR			0x042C
+#define VDISPR			0x0430
+#define VFUEN			0x0434
+
+/* DBI-B Host Registers */
+#define DBIBCTRL		0x0440
+
+/* SPI Master Registers */
+#define SPICMR			0x0450
+#define SPITCR			0x0454
+
+/* System Controller Registers */
+#define SYSSTAT			0x0460
+#define SYSCTRL			0x0464
+#define SYSPLL1			0x0468
+#define SYSPLL2			0x046C
+#define SYSPLL3			0x0470
+#define SYSPMCTRL		0x047C
+
+/* GPIO Registers */
+#define GPIOC			0x0480
+#define GPIOO			0x0484
+#define GPIOI			0x0488
+
+/* I2C Registers */
+#define I2CCLKCTRL		0x0490
+
+/* Chip/Rev Registers */
+#define IDREG			0x04A0
+
+/* Debug Registers */
+#define WCMDQUEUE		0x0500
+#define RCMDQUEUE		0x0504
+
+struct rpi_touchscreen {
+	struct drm_panel base;
+	struct mipi_dsi_device *dsi;
+	struct i2c_client *i2c;
+};
+
+static const struct drm_display_mode rpi_touchscreen_modes[] = {
+	{
+		/* Modeline comes from the Raspberry Pi firmware, with HFP=1
+		 * plugged in and clock re-computed from that.
+		 */
+		.clock = 25979400 / 1000,
+		.hdisplay = 800,
+		.hsync_start = 800 + 1,
+		.hsync_end = 800 + 1 + 2,
+		.htotal = 800 + 1 + 2 + 46,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 2,
+		.vtotal = 480 + 7 + 2 + 21,
+		.vrefresh = 60,
+	},
+};
+
+static struct rpi_touchscreen *panel_to_ts(struct drm_panel *panel)
+{
+	return container_of(panel, struct rpi_touchscreen, base);
+}
+
+static u8 rpi_touchscreen_i2c_read(struct rpi_touchscreen *ts, u8 reg)
+{
+	return i2c_smbus_read_byte_data(ts->i2c, reg);
+}
+
+static void rpi_touchscreen_i2c_write(struct rpi_touchscreen *ts,
+				      u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
+	if (ret)
+		dev_err(&ts->dsi->dev, "I2C write failed: %d\n", ret);
+}
+
+static int rpi_touchscreen_write(struct rpi_touchscreen *ts, u16 reg, u32 val)
+{
+#if 0
+	/* The firmware uses LP DSI transactions like this to bring up
+	 * the hardware, which should be faster than using I2C to then
+	 * pass to the Toshiba.  However, I was unable to get it to
+	 * work.
+	 */
+	u8 msg[] = {
+		reg,
+		reg >> 8,
+		val,
+		val >> 8,
+		val >> 16,
+		val >> 24,
+	};
+
+	mipi_dsi_dcs_write_buffer(ts->dsi, msg, sizeof(msg));
+#else
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRH, reg >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRL, reg);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEH, val >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEL, val);
+#endif
+
+	return 0;
+}
+
+static int rpi_touchscreen_disable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+	udelay(1);
+
+	return 0;
+}
+
+static int rpi_touchscreen_noop(struct drm_panel *panel)
+{
+	return 0;
+}
+
+static int rpi_touchscreen_enable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+	int i;
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
+	/* Wait for nPWRDWN to go low to indicate poweron is done. */
+	for (i = 0; i < 100; i++) {
+		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
+			break;
+	}
+
+	rpi_touchscreen_write(ts, DSI_LANEENABLE,
+			      DSI_LANEENABLE_CLOCK |
+			      DSI_LANEENABLE_D0);
+	rpi_touchscreen_write(ts, PPI_D0S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D1S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D0S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_D1S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_LPTXTIMECNT, 0x03);
+
+	rpi_touchscreen_write(ts, SPICMR, 0x00);
+	rpi_touchscreen_write(ts, LCDCTRL, 0x00100150);
+	rpi_touchscreen_write(ts, SYSCTRL, 0x040f);
+	msleep(100);
+
+	rpi_touchscreen_write(ts, PPI_STARTPPI, 0x01);
+	rpi_touchscreen_write(ts, DSI_STARTDSI, 0x01);
+	msleep(100);
+
+	/* Turn on the backlight. */
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+
+	/* Default to the same orientation as the closed source
+	 * firmware used for the panel.  Runtime rotation
+	 * configuration will be supported using VC4's plane
+	 * orientation bits.
+	 */
+	rpi_touchscreen_i2c_write(ts, REG_PORTA, BIT(2));
+
+	return 0;
+}
+
+static int rpi_touchscreen_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct drm_device *drm = panel->drm;
+	unsigned int i, num = 0;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_touchscreen_modes); i++) {
+		const struct drm_display_mode *m = &rpi_touchscreen_modes[i];
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_duplicate(drm, m);
+		if (!mode) {
+			dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, m->vrefresh);
+			continue;
+		}
+
+		mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (i == 0)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+
+		drm_mode_probed_add(connector, mode);
+		num++;
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = 154;
+	connector->display_info.height_mm = 86;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return num;
+}
+
+static const struct drm_panel_funcs rpi_touchscreen_funcs = {
+	.disable = rpi_touchscreen_disable,
+	.unprepare = rpi_touchscreen_noop,
+	.prepare = rpi_touchscreen_noop,
+	.enable = rpi_touchscreen_enable,
+	.get_modes = rpi_touchscreen_get_modes,
+};
+
+static int rpi_touchscreen_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct rpi_touchscreen *ts;
+	struct device_node *dsi_host_node, *endpoint;
+	struct mipi_dsi_host *host;
+	int ret, ver;
+	struct mipi_dsi_device_info info = {
+		.type = RPI_DSI_DRIVER_NAME,
+		.channel = 0,
+		.node = NULL,
+	};
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, ts);
+
+	ts->i2c = i2c;
+
+	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
+	if (ver < 0) {
+		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
+		return -ENODEV;
+	}
+
+	switch (ver) {
+	case 0xde: /* ver 1 */
+	case 0xc3: /* ver 2 */
+		break;
+	default:
+		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
+		return -ENODEV;
+	}
+
+	/* Turn off at boot, so we can cleanly sequence powering on. */
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	dsi_host_node = of_graph_get_remote_port_parent(endpoint);
+	of_node_put(endpoint);
+
+	host = of_find_mipi_dsi_host_by_node(dsi_host_node);
+	of_node_put(dsi_host_node);
+
+	if (!host) {
+		dev_dbg(dev, "MIPI DSI host not registered yet\n");
+		return -EPROBE_DEFER;
+	}
+
+	ts->dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(ts->dsi)) {
+		dev_err(dev, "DSI device registration failed: %ld\n",
+			PTR_ERR(ts->dsi));
+		return PTR_ERR(ts->dsi);
+	}
+
+	ts->dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
+			       MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			       MIPI_DSI_MODE_LPM);
+	ts->dsi->format = MIPI_DSI_FMT_RGB888;
+	ts->dsi->lanes = 1;
+
+	ts->base.dev = &ts->dsi->dev;
+	ts->base.funcs = &rpi_touchscreen_funcs;
+
+	ret = drm_panel_add(&ts->base);
+	if (ret)
+		return ret;
+
+	ret = mipi_dsi_attach(ts->dsi);
+	if (ret) {
+		dev_err(dev, "failed to attach dsi to host\n");
+		drm_panel_remove(&ts->base);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rpi_touchscreen_remove(struct i2c_client *i2c)
+{
+	struct rpi_touchscreen *ts = i2c_get_clientdata(i2c);
+
+	mipi_dsi_detach(ts->dsi);
+
+	drm_panel_remove(&ts->base);
+
+	mipi_dsi_device_unregister(ts->dsi);
+	kfree(ts->dsi);
+
+	return 0;
+}
+
+static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = {
+	.driver.name = RPI_DSI_DRIVER_NAME,
+};
+
+static const struct of_device_id rpi_touchscreen_of_ids[] = {
+	{ .compatible = "raspberrypi,7inch-touchscreen-panel" },
+	{ } /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, rpi_touchscreen_of_ids);
+
+static struct i2c_driver rpi_touchscreen_driver = {
+	.driver = {
+		.name = "rpi_touchscreen",
+		.of_match_table = rpi_touchscreen_of_ids,
+	},
+	.probe = rpi_touchscreen_probe,
+	.remove = rpi_touchscreen_remove,
+};
+
+static int __init rpi_touchscreen_init(void)
+{
+	mipi_dsi_driver_register(&rpi_touchscreen_dsi_driver);
+	return i2c_add_driver(&rpi_touchscreen_driver);
+}
+module_init(rpi_touchscreen_init);
+
+static void __exit rpi_touchscreen_exit(void)
+{
+	i2c_del_driver(&rpi_touchscreen_driver);
+	mipi_dsi_driver_unregister(&rpi_touchscreen_dsi_driver);
+}
+module_exit(rpi_touchscreen_exit);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi 7-inch touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

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

[PATCH 7/7] ARM: dts: bcm2835: Enable the Raspberry Pi touchscreen panel.

[Eric Anholt]

This commit is not intended to be merged.  Instead we will use
overlays to enable the panel, and this commit is just a demo of how
things get wired up.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835-rpi-b-plus.dts         |  5 ++++
 arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts         |  5 ++++
 arch/arm/boot/dts/bcm2835-rpi-b.dts              |  5 ++++
 arch/arm/boot/dts/bcm2836-rpi-2-b.dts            |  5 ++++
 arch/arm/boot/dts/bcm283x.dtsi                   | 30 +++++++++++++++++++++++-
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts |  5 ++++
 6 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
index 432088ebb0a1..79f433eacb8b 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
@@ -101,3 +101,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+};
diff --git a/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts b/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts
index 4133bc2cd9be..145e0e4c2c7f 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts
@@ -94,3 +94,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 2 0
+		 &gpio 3 0>;
+};
diff --git a/arch/arm/boot/dts/bcm2835-rpi-b.dts b/arch/arm/boot/dts/bcm2835-rpi-b.dts
index 4d56fe3006b0..5b3f85883f0c 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b.dts
@@ -89,3 +89,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_HIGH>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 0 0
+		 &gpio 1 0>;
+};
diff --git a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
index bf19e8cfb9e6..aa63cc29eca8 100644
--- a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
+++ b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
@@ -39,3 +39,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+};
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 35cea3fcaf5c..682f11a4fc91 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -505,7 +505,11 @@
 					     "dsi1_ddr2",
 					     "dsi1_ddr";
 
-			status = "disabled";
+			port {
+				dsi_out_port: endpoint {
+					remote-endpoint = <&panel_dsi_port>;
+				};
+			};
 		};
 
 		i2c1: i2c@7e804000 {
@@ -575,6 +579,30 @@
 		vc4: gpu {
 			compatible = "brcm,bcm2835-vc4";
 		};
+
+		i2c_dsi: i2c {
+			/* We have to use i2c-gpio because the
+			 * firmware is also polling another device
+			 * using the only hardware I2C bus that could
+			 * connect to these pins.
+			 */
+			compatible = "i2c-gpio";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			gpios = <&gpio 28 0
+				 &gpio 29 0>;
+
+			lcd@45 {
+				compatible = "raspberrypi,7inch-touchscreen-panel";
+				reg = <0x45>;
+
+				port {
+					panel_dsi_port: endpoint {
+						remote-endpoint = <&dsi_out_port>;
+					};
+				};
+			};
+		};
 	};
 
 	clocks {
diff --git a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
index c309633a1e87..c11026c6c8b4 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
@@ -22,3 +22,8 @@
 &uart1 {
 	status = "okay";
 };
+
+&i2c_dsi {
+	gpios = <&gpio 44 0
+		 &gpio 45 0>;
+};
-- 
2.11.0

[PATCH 7/7] ARM: dts: bcm2835: Enable the Raspberry Pi touchscreen panel.

[Eric Anholt]

This commit is not intended to be merged.  Instead we will use
overlays to enable the panel, and this commit is just a demo of how
things get wired up.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm/boot/dts/bcm2835-rpi-b-plus.dts         |  5 ++++
 arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts         |  5 ++++
 arch/arm/boot/dts/bcm2835-rpi-b.dts              |  5 ++++
 arch/arm/boot/dts/bcm2836-rpi-2-b.dts            |  5 ++++
 arch/arm/boot/dts/bcm283x.dtsi                   | 30 +++++++++++++++++++++++-
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts |  5 ++++
 6 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
index 432088ebb0a1..79f433eacb8b 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
@@ -101,3 +101,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+};
diff --git a/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts b/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts
index 4133bc2cd9be..145e0e4c2c7f 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b-rev2.dts
@@ -94,3 +94,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 2 0
+		 &gpio 3 0>;
+};
diff --git a/arch/arm/boot/dts/bcm2835-rpi-b.dts b/arch/arm/boot/dts/bcm2835-rpi-b.dts
index 4d56fe3006b0..5b3f85883f0c 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b.dts
@@ -89,3 +89,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_HIGH>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 0 0
+		 &gpio 1 0>;
+};
diff --git a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
index bf19e8cfb9e6..aa63cc29eca8 100644
--- a/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
+++ b/arch/arm/boot/dts/bcm2836-rpi-2-b.dts
@@ -39,3 +39,8 @@
 &hdmi {
 	hpd-gpios = <&gpio 46 GPIO_ACTIVE_LOW>;
 };
+
+&i2c_dsi {
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+};
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 35cea3fcaf5c..682f11a4fc91 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -505,7 +505,11 @@
 					     "dsi1_ddr2",
 					     "dsi1_ddr";
 
-			status = "disabled";
+			port {
+				dsi_out_port: endpoint {
+					remote-endpoint = <&panel_dsi_port>;
+				};
+			};
 		};
 
 		i2c1: i2c@7e804000 {
@@ -575,6 +579,30 @@
 		vc4: gpu {
 			compatible = "brcm,bcm2835-vc4";
 		};
+
+		i2c_dsi: i2c {
+			/* We have to use i2c-gpio because the
+			 * firmware is also polling another device
+			 * using the only hardware I2C bus that could
+			 * connect to these pins.
+			 */
+			compatible = "i2c-gpio";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			gpios = <&gpio 28 0
+				 &gpio 29 0>;
+
+			lcd@45 {
+				compatible = "raspberrypi,7inch-touchscreen-panel";
+				reg = <0x45>;
+
+				port {
+					panel_dsi_port: endpoint {
+						remote-endpoint = <&dsi_out_port>;
+					};
+				};
+			};
+		};
 	};
 
 	clocks {
diff --git a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
index c309633a1e87..c11026c6c8b4 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
@@ -22,3 +22,8 @@
 &uart1 {
 	status = "okay";
 };
+
+&i2c_dsi {
+	gpios = <&gpio 44 0
+		 &gpio 45 0>;
+};
-- 
2.11.0

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