diff mbox series

[v3,45/50] drm/omap: dpi: Register a drm_bridge

Message ID 20191210225750.15709-46-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm/omap: Replace custom display drivers with drm_bridge and drm_panel | expand

Commit Message

Laurent Pinchart Dec. 10, 2019, 10:57 p.m. UTC 
In order to integrate with a chain of drm_bridge, the internal DPI
output has to expose its operations through the drm_bridge API.
Register a bridge at initialisation time to do so and remove the
omap_dss_device operations that are now unused.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Unregister bridge if port initialisation fails
---
 drivers/gpu/drm/omapdrm/dss/dpi.c | 197 ++++++++++++++++++------------
 1 file changed, 119 insertions(+), 78 deletions(-)

Comments

Tomi Valkeinen Dec. 19, 2019, 9:21 a.m. UTC | #1 
On 11/12/2019 00:57, Laurent Pinchart wrote:
> In order to integrate with a chain of drm_bridge, the internal DPI
> output has to expose its operations through the drm_bridge API.
> Register a bridge at initialisation time to do so and remove the
> omap_dss_device operations that are now unused.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Unregister bridge if port initialisation fails
> ---
>   drivers/gpu/drm/omapdrm/dss/dpi.c | 197 ++++++++++++++++++------------
>   1 file changed, 119 insertions(+), 78 deletions(-)

I don't think DPI is really a bridge, as it's really just direct output from the DISPC (level 
shifted). But that probably doesn't matter, and bridge is a good way to manage the DPI output.

> +static void dpi_bridge_mode_set(struct drm_bridge *bridge,
> +				 const struct drm_display_mode *mode,
> +				 const struct drm_display_mode *adjusted_mode)
> +{
> +	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
> +
> +	mutex_lock(&dpi->lock);
> +	dpi->pixelclock = adjusted_mode->clock * 1000;
> +	mutex_unlock(&dpi->lock);
> +}

What's the lock protecting? Why do we lock here, but not e.g. in mode_fixup?

Do we ever get drm_bridge_funcs calls from multiple threads at the same time? Is there a difference 
between having a single DPI output, or multiple DPI outputs (i.e. can two different DPI outputs get 
calls simultaneously?).

  Tomi
Laurent Pinchart Dec. 19, 2019, 9:40 a.m. UTC | #2 
Hi Tomi,

On Thu, Dec 19, 2019 at 11:21:03AM +0200, Tomi Valkeinen wrote:
> On 11/12/2019 00:57, Laurent Pinchart wrote:
> > In order to integrate with a chain of drm_bridge, the internal DPI
> > output has to expose its operations through the drm_bridge API.
> > Register a bridge at initialisation time to do so and remove the
> > omap_dss_device operations that are now unused.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> > 
> > - Unregister bridge if port initialisation fails
> > ---
> >   drivers/gpu/drm/omapdrm/dss/dpi.c | 197 ++++++++++++++++++------------
> >   1 file changed, 119 insertions(+), 78 deletions(-)
> 
> I don't think DPI is really a bridge, as it's really just direct
> output from the DISPC (level shifted). But that probably doesn't
> matter, and bridge is a good way to manage the DPI output.

No disagreement as long as we can use drm_bridge :-)

> > +static void dpi_bridge_mode_set(struct drm_bridge *bridge,
> > +				 const struct drm_display_mode *mode,
> > +				 const struct drm_display_mode *adjusted_mode)
> > +{
> > +	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
> > +
> > +	mutex_lock(&dpi->lock);
> > +	dpi->pixelclock = adjusted_mode->clock * 1000;
> > +	mutex_unlock(&dpi->lock);
> > +}
> 
> What's the lock protecting? Why do we lock here, but not e.g. in
> mode_fixup?

Not much anymore it seems.

> Do we ever get drm_bridge_funcs calls from multiple threads at the
> same time? Is there a difference between having a single DPI output,
> or multiple DPI outputs (i.e. can two different DPI outputs get calls
> simultaneously?).

I'll drop the lock, it's not needed. The core should serialize calls to
the bridge, at least per output. For different outputs, there's a
possibility I believe of atomic commits being handled concurrently if
they don't share any resource.
Tomi Valkeinen Dec. 19, 2019, 10:01 a.m. UTC | #3 
On 19/12/2019 11:40, Laurent Pinchart wrote:

>> Do we ever get drm_bridge_funcs calls from multiple threads at the
>> same time? Is there a difference between having a single DPI output,
>> or multiple DPI outputs (i.e. can two different DPI outputs get calls
>> simultaneously?).
> 
> I'll drop the lock, it's not needed. The core should serialize calls to
> the bridge, at least per output. For different outputs, there's a
> possibility I believe of atomic commits being handled concurrently if
> they don't share any resource.

I don't think we do much locking with resource handling. E.g. we have single registers with mux 
settings for multiple outputs. If DPI (or any other dss output) gets called concurrently for 
different outputs, we might get a race.

I wonder if that concurrency is opt-in...

  Tomi
Laurent Pinchart Dec. 19, 2019, 10:03 a.m. UTC | #4 
On Thu, Dec 19, 2019 at 12:01:34PM +0200, Tomi Valkeinen wrote:
> On 19/12/2019 11:40, Laurent Pinchart wrote:
> >> Do we ever get drm_bridge_funcs calls from multiple threads at the
> >> same time? Is there a difference between having a single DPI output,
> >> or multiple DPI outputs (i.e. can two different DPI outputs get calls
> >> simultaneously?).
> > 
> > I'll drop the lock, it's not needed. The core should serialize calls to
> > the bridge, at least per output. For different outputs, there's a
> > possibility I believe of atomic commits being handled concurrently if
> > they don't share any resource.
> 
> I don't think we do much locking with resource handling. E.g. we have single registers with mux 
> settings for multiple outputs. If DPI (or any other dss output) gets called concurrently for 
> different outputs, we might get a race.
> 
> I wonder if that concurrency is opt-in...

It's at least opt-out in the sense that we can acquire all resources in
our top-level .atomic_check() implementation if we want to. Of course
the best option would be to handle locking correcly in the core :-) With
this rework done, I want to focus on Sebastian's DSI move to drm_bridge,
and then remove lots of dead code. I think a cleanup pass in the DISPC
would be useful after that.
Tomi Valkeinen Dec. 19, 2019, 10:15 a.m. UTC | #5 
On 19/12/2019 12:03, Laurent Pinchart wrote:
> On Thu, Dec 19, 2019 at 12:01:34PM +0200, Tomi Valkeinen wrote:
>> On 19/12/2019 11:40, Laurent Pinchart wrote:
>>>> Do we ever get drm_bridge_funcs calls from multiple threads at the
>>>> same time? Is there a difference between having a single DPI output,
>>>> or multiple DPI outputs (i.e. can two different DPI outputs get calls
>>>> simultaneously?).
>>>
>>> I'll drop the lock, it's not needed. The core should serialize calls to
>>> the bridge, at least per output. For different outputs, there's a
>>> possibility I believe of atomic commits being handled concurrently if
>>> they don't share any resource.
>>
>> I don't think we do much locking with resource handling. E.g. we have single registers with mux
>> settings for multiple outputs. If DPI (or any other dss output) gets called concurrently for
>> different outputs, we might get a race.
>>
>> I wonder if that concurrency is opt-in...
> 
> It's at least opt-out in the sense that we can acquire all resources in
> our top-level .atomic_check() implementation if we want to. Of course
> the best option would be to handle locking correcly in the core :-) With

I agree, but I wonder if that's just a lot of work and possibilities for complex to find bugs, for 
little benefit.

> this rework done, I want to focus on Sebastian's DSI move to drm_bridge,
> and then remove lots of dead code. I think a cleanup pass in the DISPC
> would be useful after that.

I agree here too, without any buts.

  Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c
index c167bd1116ec..e228766f613d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dpi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dpi.c
@@ -21,6 +21,8 @@ 
 #include <linux/string.h>
 #include <linux/sys_soc.h>
 
+#include <drm/drm_bridge.h>
+
 #include "dss.h"
 #include "omapdss.h"
 
@@ -41,12 +43,10 @@  struct dpi_data {
 	int data_lines;
 
 	struct omap_dss_device output;
+	struct drm_bridge bridge;
 };
 
-static struct dpi_data *dpi_get_data_from_dssdev(struct omap_dss_device *dssdev)
-{
-	return container_of(dssdev, struct dpi_data, output);
-}
+#define drm_bridge_to_dpi(bridge) container_of(bridge, struct dpi_data, bridge)
 
 /* -----------------------------------------------------------------------------
  * Clock Handling and PLL
@@ -354,6 +354,32 @@  static void dpi_config_lcd_manager(struct dpi_data *dpi)
 	dss_mgr_set_lcd_config(&dpi->output, &dpi->mgr_config);
 }
 
+static int dpi_clock_update(struct dpi_data *dpi, unsigned long *clock)
+{
+	int lck_div, pck_div;
+	unsigned long fck;
+	struct dpi_clk_calc_ctx ctx;
+
+	if (dpi->pll) {
+		if (!dpi_pll_clk_calc(dpi, *clock, &ctx))
+			return -EINVAL;
+
+		fck = ctx.pll_cinfo.clkout[ctx.clkout_idx];
+	} else {
+		if (!dpi_dss_clk_calc(dpi, *clock, &ctx))
+			return -EINVAL;
+
+		fck = ctx.fck;
+	}
+
+	lck_div = ctx.dispc_cinfo.lck_div;
+	pck_div = ctx.dispc_cinfo.pck_div;
+
+	*clock = fck / lck_div / pck_div;
+
+	return 0;
+}
+
 static int dpi_verify_pll(struct dss_pll *pll)
 {
 	int r;
@@ -391,29 +417,76 @@  static void dpi_init_pll(struct dpi_data *dpi)
 }
 
 /* -----------------------------------------------------------------------------
- * omap_dss_device Operations
+ * DRM Bridge Operations
  */
 
-static int dpi_connect(struct omap_dss_device *src,
-		       struct omap_dss_device *dst)
+static int dpi_bridge_attach(struct drm_bridge *bridge,
+			     enum drm_bridge_attach_flags flags)
 {
-	struct dpi_data *dpi = dpi_get_data_from_dssdev(dst);
+	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
+
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
+		return -EINVAL;
 
 	dpi_init_pll(dpi);
 
-	return omapdss_device_connect(dst->dss, dst, dst->next);
+	return drm_bridge_attach(bridge->encoder, dpi->output.next_bridge,
+				 bridge, flags);
 }
 
-static void dpi_disconnect(struct omap_dss_device *src,
-			   struct omap_dss_device *dst)
+static enum drm_mode_status
+dpi_bridge_mode_valid(struct drm_bridge *bridge,
+		       const struct drm_display_mode *mode)
 {
-	omapdss_device_disconnect(dst, dst->next);
+	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
+	unsigned long clock = mode->clock * 1000;
+	int ret;
+
+	if (mode->hdisplay % 8 != 0)
+		return MODE_BAD_WIDTH;
+
+	if (mode->clock == 0)
+		return MODE_NOCLOCK;
+
+	ret = dpi_clock_update(dpi, &clock);
+	if (ret < 0)
+		return MODE_CLOCK_RANGE;
+
+	return MODE_OK;
 }
 
-static void dpi_display_enable(struct omap_dss_device *dssdev)
+static bool dpi_bridge_mode_fixup(struct drm_bridge *bridge,
+				   const struct drm_display_mode *mode,
+				   struct drm_display_mode *adjusted_mode)
 {
-	struct dpi_data *dpi = dpi_get_data_from_dssdev(dssdev);
-	struct omap_dss_device *out = &dpi->output;
+	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
+	unsigned long clock = mode->clock * 1000;
+	int ret;
+
+	ret = dpi_clock_update(dpi, &clock);
+	if (ret < 0)
+		return false;
+
+	adjusted_mode->clock = clock / 1000;
+
+	return true;
+}
+
+static void dpi_bridge_mode_set(struct drm_bridge *bridge,
+				 const struct drm_display_mode *mode,
+				 const struct drm_display_mode *adjusted_mode)
+{
+	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
+
+	mutex_lock(&dpi->lock);
+	dpi->pixelclock = adjusted_mode->clock * 1000;
+	mutex_unlock(&dpi->lock);
+}
+
+static void dpi_bridge_enable(struct drm_bridge *bridge,
+			       struct drm_atomic_state *state)
+{
+	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
 	int r;
 
 	mutex_lock(&dpi->lock);
@@ -428,7 +501,7 @@  static void dpi_display_enable(struct omap_dss_device *dssdev)
 	if (r)
 		goto err_get_dispc;
 
-	r = dss_dpi_select_source(dpi->dss, dpi->id, out->dispc_channel);
+	r = dss_dpi_select_source(dpi->dss, dpi->id, dpi->output.dispc_channel);
 	if (r)
 		goto err_src_sel;
 
@@ -468,9 +541,10 @@  static void dpi_display_enable(struct omap_dss_device *dssdev)
 	mutex_unlock(&dpi->lock);
 }
 
-static void dpi_display_disable(struct omap_dss_device *dssdev)
+static void dpi_bridge_disable(struct drm_bridge *bridge,
+				struct drm_atomic_state *state)
 {
-	struct dpi_data *dpi = dpi_get_data_from_dssdev(dssdev);
+	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
 
 	mutex_lock(&dpi->lock);
 
@@ -490,70 +564,32 @@  static void dpi_display_disable(struct omap_dss_device *dssdev)
 	mutex_unlock(&dpi->lock);
 }
 
-static int dpi_check_timings(struct omap_dss_device *dssdev,
-			     struct drm_display_mode *mode)
-{
-	struct dpi_data *dpi = dpi_get_data_from_dssdev(dssdev);
-	int lck_div, pck_div;
-	unsigned long fck;
-	unsigned long pck;
-	struct dpi_clk_calc_ctx ctx;
-	bool ok;
-
-	if (mode->hdisplay % 8 != 0)
-		return -EINVAL;
-
-	if (mode->clock == 0)
-		return -EINVAL;
-
-	if (dpi->pll) {
-		ok = dpi_pll_clk_calc(dpi, mode->clock * 1000, &ctx);
-		if (!ok)
-			return -EINVAL;
-
-		fck = ctx.pll_cinfo.clkout[ctx.clkout_idx];
-	} else {
-		ok = dpi_dss_clk_calc(dpi, mode->clock * 1000, &ctx);
-		if (!ok)
-			return -EINVAL;
-
-		fck = ctx.fck;
-	}
-
-	lck_div = ctx.dispc_cinfo.lck_div;
-	pck_div = ctx.dispc_cinfo.pck_div;
-
-	pck = fck / lck_div / pck_div;
+static const struct drm_bridge_funcs dpi_bridge_funcs = {
+	.attach = dpi_bridge_attach,
+	.mode_valid = dpi_bridge_mode_valid,
+	.mode_fixup = dpi_bridge_mode_fixup,
+	.mode_set = dpi_bridge_mode_set,
+	.atomic_enable = dpi_bridge_enable,
+	.atomic_disable = dpi_bridge_disable,
+};
 
-	mode->clock = pck / 1000;
+static void dpi_bridge_init(struct dpi_data *dpi)
+{
+	dpi->bridge.funcs = &dpi_bridge_funcs;
+	dpi->bridge.of_node = dpi->pdev->dev.of_node;
+	dpi->bridge.type = DRM_MODE_CONNECTOR_DPI;
 
-	return 0;
+	drm_bridge_add(&dpi->bridge);
 }
 
-static void dpi_set_timings(struct omap_dss_device *dssdev,
-			    const struct drm_display_mode *mode)
+static void dpi_bridge_cleanup(struct dpi_data *dpi)
 {
-	struct dpi_data *dpi = dpi_get_data_from_dssdev(dssdev);
-
-	DSSDBG("dpi_set_timings\n");
-
-	mutex_lock(&dpi->lock);
-
-	dpi->pixelclock = mode->clock * 1000;
-
-	mutex_unlock(&dpi->lock);
+	drm_bridge_remove(&dpi->bridge);
 }
 
-static const struct omap_dss_device_ops dpi_ops = {
-	.connect = dpi_connect,
-	.disconnect = dpi_disconnect,
-
-	.enable = dpi_display_enable,
-	.disable = dpi_display_disable,
-
-	.check_timings = dpi_check_timings,
-	.set_timings = dpi_set_timings,
-};
+/* -----------------------------------------------------------------------------
+ * Initialisation and Cleanup
+ */
 
 /*
  * Return a hardcoded channel for the DPI output. This should work for
@@ -597,6 +633,8 @@  static int dpi_init_output_port(struct dpi_data *dpi, struct device_node *port)
 	u32 port_num = 0;
 	int r;
 
+	dpi_bridge_init(dpi);
+
 	of_property_read_u32(port, "reg", &port_num);
 	dpi->id = port_num <= 2 ? port_num : 0;
 
@@ -618,12 +656,13 @@  static int dpi_init_output_port(struct dpi_data *dpi, struct device_node *port)
 	out->type = OMAP_DISPLAY_TYPE_DPI;
 	out->dispc_channel = dpi_get_channel(dpi);
 	out->of_port = port_num;
-	out->ops = &dpi_ops;
 	out->owner = THIS_MODULE;
 
-	r = omapdss_device_init_output(out, NULL);
-	if (r < 0)
+	r = omapdss_device_init_output(out, &dpi->bridge);
+	if (r < 0) {
+		dpi_bridge_cleanup(dpi);
 		return r;
+	}
 
 	omapdss_device_register(out);
 
@@ -637,6 +676,8 @@  static void dpi_uninit_output_port(struct device_node *port)
 
 	omapdss_device_unregister(out);
 	omapdss_device_cleanup_output(out);
+
+	dpi_bridge_cleanup(dpi);
 }
 
 /* -----------------------------------------------------------------------------