| Message ID | 20230329131929.1328612-3-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi, The patch prefix should be drm/sun4i: On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote: > Convert the encoder to bridge driver in order to standardize on a > single API by supporting all varients of downstream bridge devices. Which variant, and why do we need to convert to a bridge to support all of them? > The drm_encoder can't be removed as it's exposed to userspace, so it > then becomes a dumb encoder, without any operation implemented. > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge. > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> [...] > +static const struct component_ops sun6i_dsi_ops; > + > static int sun6i_dsi_attach(struct mipi_dsi_host *host, > struct mipi_dsi_device *device) > { > struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); > - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); That one looks unrelated. Why do you need that change? Maxime
On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > The patch prefix should be drm/sun4i: I did follow my previous prefix, I will update this. > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote: > > Convert the encoder to bridge driver in order to standardize on a > > single API by supporting all varients of downstream bridge devices. > > Which variant, and why do we need to convert to a bridge to support all of them? Downstream bridge variants like DSI panel, DSI bridge and I2C-Configured DSI bridges. Bridge conversion would be required for the DSI host to access the more variety and complex downstream bridges in a standardized bridge chain way which is indeed complex for encoder driven DSI hosts. > > > The drm_encoder can't be removed as it's exposed to userspace, so it > > then becomes a dumb encoder, without any operation implemented. > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge. > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > [...] > > > +static const struct component_ops sun6i_dsi_ops; > > + > > static int sun6i_dsi_attach(struct mipi_dsi_host *host, > > struct mipi_dsi_device *device) > > { > > struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); > > - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); > > That one looks unrelated. Why do you need that change? This was replaced with drmm_of_dsi_get_bridge for lookup of both panel and bridge. I think I will separate this into another patch. Thanks, Jagan.
On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote: > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > Hi, > > > > The patch prefix should be drm/sun4i: > > I did follow my previous prefix, I will update this. > > > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote: > > > Convert the encoder to bridge driver in order to standardize on a > > > single API by supporting all varients of downstream bridge devices. > > > > Which variant, and why do we need to convert to a bridge to support all of them? > > Downstream bridge variants like DSI panel, DSI bridge and > I2C-Configured DSI bridges. Bridge conversion would be required for > the DSI host to access the more variety and complex downstream bridges > in a standardized bridge chain way which is indeed complex for encoder > driven DSI hosts. > > > > > > The drm_encoder can't be removed as it's exposed to userspace, so it > > > then becomes a dumb encoder, without any operation implemented. > > > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge. > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > [...] > > > > > +static const struct component_ops sun6i_dsi_ops; > > > + > > > static int sun6i_dsi_attach(struct mipi_dsi_host *host, > > > struct mipi_dsi_device *device) > > > { > > > struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); > > > - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); > > > > That one looks unrelated. Why do you need that change? > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel > and bridge. I think I will separate this into another patch. So, it looks to me that you're doing two (unrelated) things in that patch: - You modify the existing driver to be a bridge - And you support downstream device being bridges. Both are orthogonal, can (and should!) be done separately, and I'm pretty sure you don't actually need to do the former at all. Maxime
On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote: > > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > Hi, > > > > > > The patch prefix should be drm/sun4i: > > > > I did follow my previous prefix, I will update this. > > > > > > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote: > > > > Convert the encoder to bridge driver in order to standardize on a > > > > single API by supporting all varients of downstream bridge devices. > > > > > > Which variant, and why do we need to convert to a bridge to support all of them? > > > > Downstream bridge variants like DSI panel, DSI bridge and > > I2C-Configured DSI bridges. Bridge conversion would be required for > > the DSI host to access the more variety and complex downstream bridges > > in a standardized bridge chain way which is indeed complex for encoder > > driven DSI hosts. > > > > > > > > > The drm_encoder can't be removed as it's exposed to userspace, so it > > > > then becomes a dumb encoder, without any operation implemented. > > > > > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge. > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > > [...] > > > > > > > +static const struct component_ops sun6i_dsi_ops; > > > > + > > > > static int sun6i_dsi_attach(struct mipi_dsi_host *host, > > > > struct mipi_dsi_device *device) > > > > { > > > > struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); > > > > - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); > > > > > > That one looks unrelated. Why do you need that change? > > > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel > > and bridge. I think I will separate this into another patch. > > So, it looks to me that you're doing two (unrelated) things in that patch: Correct. > > - You modify the existing driver to be a bridge Yes, Convert to bridge driver - register drm_bridge_add and replace encoder ops with bridge ops. > > - And you support downstream device being bridges. Yes, Support the downstream bridge. (If I'm correct we can still use encoder ops with this). If we see the hierarchy of support it would 1. support the downstream bridge. 2. convert to the bridge driver. > > Both are orthogonal, can (and should!) be done separately, and I'm > pretty sure you don't actually need to do the former at all. Do you mean converting to bridge driver is not needed? Thanks, Jagan.
On Thu, Mar 30, 2023 at 12:15:49PM +0530, Jagan Teki wrote: > On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote: > > > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > Hi, > > > > > > > > The patch prefix should be drm/sun4i: > > > > > > I did follow my previous prefix, I will update this. > > > > > > > > > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote: > > > > > Convert the encoder to bridge driver in order to standardize on a > > > > > single API by supporting all varients of downstream bridge devices. > > > > > > > > Which variant, and why do we need to convert to a bridge to support all of them? > > > > > > Downstream bridge variants like DSI panel, DSI bridge and > > > I2C-Configured DSI bridges. Bridge conversion would be required for > > > the DSI host to access the more variety and complex downstream bridges > > > in a standardized bridge chain way which is indeed complex for encoder > > > driven DSI hosts. > > > > > > > > > > > > The drm_encoder can't be removed as it's exposed to userspace, so it > > > > > then becomes a dumb encoder, without any operation implemented. > > > > > > > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge. > > > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > > > > > > > [...] > > > > > > > > > +static const struct component_ops sun6i_dsi_ops; > > > > > + > > > > > static int sun6i_dsi_attach(struct mipi_dsi_host *host, > > > > > struct mipi_dsi_device *device) > > > > > { > > > > > struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); > > > > > - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); > > > > > > > > That one looks unrelated. Why do you need that change? > > > > > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel > > > and bridge. I think I will separate this into another patch. > > > > So, it looks to me that you're doing two (unrelated) things in that patch: > > Correct. > > > > > - You modify the existing driver to be a bridge > > Yes, Convert to bridge driver - register drm_bridge_add and replace > encoder ops with bridge ops. > > > > > - And you support downstream device being bridges. > > Yes, Support the downstream bridge. (If I'm correct we can still use > encoder ops with this). > > If we see the hierarchy of support it would > 1. support the downstream bridge. > 2. convert to the bridge driver. > > > > > Both are orthogonal, can (and should!) be done separately, and I'm > > pretty sure you don't actually need to do the former at all. > > Do you mean converting to bridge driver is not needed? Yes, and given the current state of the DCS-in-HS discussion, I even think it's does more harm than good. Maxime
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 760ff05eabf4..71951a6dc914 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -20,8 +20,8 @@ #include <linux/slab.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> #include <drm/drm_mipi_dsi.h> -#include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -713,10 +713,11 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi, return 0; } -static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) +static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_state) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; - struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); + struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode; + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); struct mipi_dsi_device *device = dsi->device; union phy_configure_opts opts = { }; struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; @@ -768,9 +769,12 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY); phy_configure(dsi->dphy, &opts); phy_power_on(dsi->dphy); +} - if (dsi->panel) - drm_panel_prepare(dsi->panel); +static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_state) +{ + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); /* * FIXME: This should be moved after the switch to HS mode. @@ -784,9 +788,6 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) * ordering on the panels I've tested it with, so I guess this * will do for now, until that IP is better understood. */ - if (dsi->panel) - drm_panel_enable(dsi->panel); - sun6i_dsi_start(dsi, DSI_START_HSC); udelay(1000); @@ -794,17 +795,13 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) sun6i_dsi_start(dsi, DSI_START_HSD); } -static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) +static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_state) { - struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); DRM_DEBUG_DRIVER("Disabling DSI output\n"); - if (dsi->panel) { - drm_panel_disable(dsi->panel); - drm_panel_unprepare(dsi->panel); - } - phy_power_off(dsi->dphy); phy_exit(dsi->dphy); @@ -813,38 +810,23 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) regulator_disable(dsi->regulator); } -static int sun6i_dsi_get_modes(struct drm_connector *connector) -{ - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); - - return drm_panel_get_modes(dsi->panel, connector); -} - -static const struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = { - .get_modes = sun6i_dsi_get_modes, -}; - -static enum drm_connector_status -sun6i_dsi_connector_detect(struct drm_connector *connector, bool force) +static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) { - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); - return dsi->panel ? connector_status_connected : - connector_status_disconnected; + return drm_bridge_attach(bridge->encoder, dsi->out_bridge, + &dsi->bridge, flags); } -static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { - .detect = sun6i_dsi_connector_detect, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = { - .disable = sun6i_dsi_encoder_disable, - .enable = sun6i_dsi_encoder_enable, +static const struct drm_bridge_funcs sun6i_mipi_dsi_bridge_funcs = { + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_pre_enable = sun6i_dsi_bridge_pre_enable, + .atomic_enable = sun6i_dsi_bridge_enable, + .atomic_disable = sun6i_dsi_bridge_disable, + .attach = sun6i_dsi_bridge_attach, }; static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi, @@ -959,20 +941,27 @@ static int sun6i_dsi_dcs_read(struct sun6i_dsi *dsi, return 1; } +static const struct component_ops sun6i_dsi_ops; + static int sun6i_dsi_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); + int ret; + + dsi->device = device; + + drm_bridge_add(&dsi->bridge); + + ret = component_add(dsi->dev, &sun6i_dsi_ops); + if (ret) { + dev_err(dsi->dev, "Couldn't register our component\n"); + return ret; + } - if (IS_ERR(panel)) - return PTR_ERR(panel); if (!dsi->drm || !dsi->drm->registered) return -EPROBE_DEFER; - dsi->panel = panel; - dsi->device = device; - drm_kms_helper_hotplug_event(dsi->drm); dev_info(host->dev, "Attached device %s\n", device->name); @@ -985,11 +974,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host, { struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); - dsi->panel = NULL; + component_del(dsi->dev, &sun6i_dsi_ops); + drm_bridge_remove(&dsi->bridge); dsi->device = NULL; - drm_kms_helper_hotplug_event(dsi->drm); - return 0; } @@ -1054,8 +1042,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master, struct sun6i_dsi *dsi = dev_get_drvdata(dev); int ret; - drm_encoder_helper_add(&dsi->encoder, - &sun6i_dsi_enc_helper_funcs); + dsi->out_bridge = drmm_of_dsi_get_bridge(drm, dev->of_node, 0, 1); + if (IS_ERR(dsi->out_bridge)) { + ret = PTR_ERR(dsi->out_bridge); + DRM_DEV_ERROR(dsi->dev, "failed to find the bridge: %d\n", ret); + return ret; + } + ret = drm_simple_encoder_init(drm, &dsi->encoder, DRM_MODE_ENCODER_DSI); if (ret) { @@ -1064,39 +1057,19 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master, } dsi->encoder.possible_crtcs = BIT(0); - drm_connector_helper_add(&dsi->connector, - &sun6i_dsi_connector_helper_funcs); - ret = drm_connector_init(drm, &dsi->connector, - &sun6i_dsi_connector_funcs, - DRM_MODE_CONNECTOR_DSI); + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); if (ret) { - dev_err(dsi->dev, - "Couldn't initialise the DSI connector\n"); - goto err_cleanup_connector; + dev_err(dsi->dev, "Couldn't attach the DSI bridge\n"); + return ret; } - drm_connector_attach_encoder(&dsi->connector, &dsi->encoder); - dsi->drm = drm; return 0; - -err_cleanup_connector: - drm_encoder_cleanup(&dsi->encoder); - return ret; -} - -static void sun6i_dsi_unbind(struct device *dev, struct device *master, - void *data) -{ - struct sun6i_dsi *dsi = dev_get_drvdata(dev); - - dsi->drm = NULL; } static const struct component_ops sun6i_dsi_ops = { .bind = sun6i_dsi_bind, - .unbind = sun6i_dsi_unbind, }; static int sun6i_dsi_probe(struct platform_device *pdev) @@ -1175,22 +1148,19 @@ static int sun6i_dsi_probe(struct platform_device *pdev) goto err_unprotect_clk; } + dsi->bridge.funcs = &sun6i_mipi_dsi_bridge_funcs; + dsi->bridge.of_node = dev->of_node; + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; + dsi->bridge.enable_next_first = true; + ret = mipi_dsi_host_register(&dsi->host); if (ret) { dev_err(dev, "Couldn't register MIPI-DSI host\n"); goto err_unprotect_clk; } - ret = component_add(&pdev->dev, &sun6i_dsi_ops); - if (ret) { - dev_err(dev, "Couldn't register our component\n"); - goto err_remove_dsi_host; - } - return 0; -err_remove_dsi_host: - mipi_dsi_host_unregister(&dsi->host); err_unprotect_clk: if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk) clk_rate_exclusive_put(dsi->mod_clk); @@ -1205,7 +1175,6 @@ static int sun6i_dsi_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; struct sun6i_dsi *dsi = dev_get_drvdata(dev); - component_del(&pdev->dev, &sun6i_dsi_ops); mipi_dsi_host_unregister(&dsi->host); if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk) clk_rate_exclusive_put(dsi->mod_clk); diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h index f1ddefe0f554..8b9263e0f4ef 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h @@ -21,9 +21,9 @@ struct sun6i_dsi_variant { }; struct sun6i_dsi { - struct drm_connector connector; struct drm_encoder encoder; struct mipi_dsi_host host; + struct drm_bridge bridge; struct clk *bus_clk; struct clk *mod_clk; @@ -35,7 +35,7 @@ struct sun6i_dsi { struct device *dev; struct mipi_dsi_device *device; struct drm_device *drm; - struct drm_panel *panel; + struct drm_bridge *out_bridge; const struct sun6i_dsi_variant *variant; }; @@ -45,10 +45,10 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host) return container_of(host, struct sun6i_dsi, host); }; -static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector) +static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge) { - return container_of(connector, struct sun6i_dsi, connector); -}; + return container_of(bridge, struct sun6i_dsi, bridge); +} static inline struct sun6i_dsi *encoder_to_sun6i_dsi(const struct drm_encoder *encoder) {
Convert the encoder to bridge driver in order to standardize on a single API by supporting all varients of downstream bridge devices. The drm_encoder can't be removed as it's exposed to userspace, so it then becomes a dumb encoder, without any operation implemented. Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v7: - drop bridge call chain - use drmm_of_dsi_get_bridge - switch to atomic bridge calls - use atomic_pre_enable and atomic_enable for previous enable Changes for v6: - support donwstream bridge - drop bridge conversion - devm_drm_of_get_bridge() require child lookup https://patchwork.kernel.org/project/dri-devel/cover/20211207054747.461029-1-jagan@amarulasolutions.com/ Changes for v5: - add atomic APIs - find host and device variant DSI devices. Changes for v4, v3: - none drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 143 ++++++++++--------------- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 10 +- 2 files changed, 61 insertions(+), 92 deletions(-)