Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API
On Tue, Mar 2, 2021 at 10:05 PM Maxime Ripard wrote: > > On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote: > > On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard wrote: > > > > > > Hi, > > > > > > On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote: > > > > Use drm_panel_bridge to replace manual panel handling code. > > > > > > > > This simplifies the driver to allows all components in the > > > > display pipeline to be treated as bridges, paving the way > > > > to generic connector handling. > > > > > > > > Use drm_bridge_connector_init to create a connector for display > > > > pipelines that use drm_bridge. > > > > > > > > This allows splitting connector operations across multiple bridges > > > > when necessary, instead of having the last bridge in the chain > > > > creating the connector and handling all connector operations > > > > internally. > > > > > > > > Signed-off-by: Jagan Teki > > > > > > Most of the code removed in that patch was actually introduced earlier > > > which feels a bit weird. Is there a reason we can't do that one first, > > > and then introduce the bridge support? > > > > This patch adds new bridge API's which requires the driver has to > > support the bridge first. > > I'm not sure what you're saying, you can definitely have a bridge > without support for a downstream bridge. I understand your point. what I'm saying here is, This patch introduces two new bridge API's devm_drm_panel_bridge_add drm_bridge_connector_init In order to add these API's the driver has to support the bridge first. All the patches before this one support bridge and this patch introduce new APIs, ie the reason we have code removed in this patch which has been added before. Okay. I think I will send the next version series till bridge conversion. Improvement patches like this can take care of later versions and even it depends on Patch v3 5/7 which indeed require a separate discussion. This way it makes less confusion. Hope it's fine for you? Jagan.
Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API
On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote:
> On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard wrote:
> >
> > Hi,
> >
> > On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
> > > Use drm_panel_bridge to replace manual panel handling code.
> > >
> > > This simplifies the driver to allows all components in the
> > > display pipeline to be treated as bridges, paving the way
> > > to generic connector handling.
> > >
> > > Use drm_bridge_connector_init to create a connector for display
> > > pipelines that use drm_bridge.
> > >
> > > This allows splitting connector operations across multiple bridges
> > > when necessary, instead of having the last bridge in the chain
> > > creating the connector and handling all connector operations
> > > internally.
> > >
> > > Signed-off-by: Jagan Teki
> >
> > Most of the code removed in that patch was actually introduced earlier
> > which feels a bit weird. Is there a reason we can't do that one first,
> > and then introduce the bridge support?
>
> This patch adds new bridge API's which requires the driver has to
> support the bridge first.
I'm not sure what you're saying, you can definitely have a bridge
without support for a downstream bridge.
Anyway, my point is that:
> ---
> Changes for v3:
> - new patch
>
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 +++--
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 --
> 2 files changed, 27 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 3cdc14daf25c..5e5d3789b3df 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -20,6 +20,7 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -769,12 +770,6 @@ static void sun6i_dsi_bridge_pre_enable(struct
> drm_bridge *bridge)
> phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
> phy_configure(dsi->dphy, );
> phy_power_on(dsi->dphy);
> -
> - if (dsi->panel)
> - drm_panel_prepare(dsi->panel);
> -
> - if (dsi->panel_bridge)
> - dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
This is added in patch 2
> }
>
> static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
> @@ -793,12 +788,6 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge
> *bridge)
>* 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);
> -
> - if (dsi->panel_bridge)
> - dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
> -
This is added in patch 2
> sun6i_dsi_start(dsi, DSI_START_HSC);
>
> udelay(1000);
> @@ -812,14 +801,6 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge
> *bridge)
>
> DRM_DEBUG_DRIVER("Disabling DSI output\n");
>
> - if (dsi->panel) {
> - drm_panel_disable(dsi->panel);
> - drm_panel_unprepare(dsi->panel);
> - } else if (dsi->panel_bridge) {
> - dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
> - }
> -
This is added in patch 2
> phy_power_off(dsi->dphy);
> phy_exit(dsi->dphy);
>
> @@ -828,63 +809,13 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge
> *bridge)
> 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)
> -{
> - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> -
> - return dsi->panel ? connector_status_connected :
> - connector_status_disconnected;
> -}
> -
> -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 int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
> - int ret;
> -
> - if (dsi->panel_bridge)
> - return drm_bridge_attach(bridge->encoder,
Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API
On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard wrote: > > Hi, > > On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote: > > Use drm_panel_bridge to replace manual panel handling code. > > > > This simplifies the driver to allows all components in the > > display pipeline to be treated as bridges, paving the way > > to generic connector handling. > > > > Use drm_bridge_connector_init to create a connector for display > > pipelines that use drm_bridge. > > > > This allows splitting connector operations across multiple bridges > > when necessary, instead of having the last bridge in the chain > > creating the connector and handling all connector operations > > internally. > > > > Signed-off-by: Jagan Teki > > Most of the code removed in that patch was actually introduced earlier > which feels a bit weird. Is there a reason we can't do that one first, > and then introduce the bridge support? This patch adds new bridge API's which requires the driver has to support the bridge first. Jagan.
Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API
Hi, On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote: > Use drm_panel_bridge to replace manual panel handling code. > > This simplifies the driver to allows all components in the > display pipeline to be treated as bridges, paving the way > to generic connector handling. > > Use drm_bridge_connector_init to create a connector for display > pipelines that use drm_bridge. > > This allows splitting connector operations across multiple bridges > when necessary, instead of having the last bridge in the chain > creating the connector and handling all connector operations > internally. > > Signed-off-by: Jagan Teki Most of the code removed in that patch was actually introduced earlier which feels a bit weird. Is there a reason we can't do that one first, and then introduce the bridge support? Maxime
[PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API
Use drm_panel_bridge to replace manual panel handling code.
This simplifies the driver to allows all components in the
display pipeline to be treated as bridges, paving the way
to generic connector handling.
Use drm_bridge_connector_init to create a connector for display
pipelines that use drm_bridge.
This allows splitting connector operations across multiple bridges
when necessary, instead of having the last bridge in the chain
creating the connector and handling all connector operations
internally.
Signed-off-by: Jagan Teki
---
Changes for v3:
- new patch
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 +++--
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 --
2 files changed, 27 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 3cdc14daf25c..5e5d3789b3df 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -20,6 +20,7 @@
#include
#include
+#include
#include
#include
#include
@@ -769,12 +770,6 @@ static void sun6i_dsi_bridge_pre_enable(struct drm_bridge
*bridge)
phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
phy_configure(dsi->dphy, );
phy_power_on(dsi->dphy);
-
- if (dsi->panel)
- drm_panel_prepare(dsi->panel);
-
- if (dsi->panel_bridge)
- dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
}
static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
@@ -793,12 +788,6 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge
*bridge)
* 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);
-
- if (dsi->panel_bridge)
- dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
-
sun6i_dsi_start(dsi, DSI_START_HSC);
udelay(1000);
@@ -812,14 +801,6 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge
*bridge)
DRM_DEBUG_DRIVER("Disabling DSI output\n");
- if (dsi->panel) {
- drm_panel_disable(dsi->panel);
- drm_panel_unprepare(dsi->panel);
- } else if (dsi->panel_bridge) {
- dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
- dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
- }
-
phy_power_off(dsi->dphy);
phy_exit(dsi->dphy);
@@ -828,63 +809,13 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge
*bridge)
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)
-{
- struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
-
- return dsi->panel ? connector_status_connected :
- connector_status_disconnected;
-}
-
-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 int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags)
{
struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
- int ret;
-
- if (dsi->panel_bridge)
- return drm_bridge_attach(bridge->encoder, dsi->panel_bridge,
NULL, 0);
-
- if (dsi->panel) {
- drm_connector_helper_add(>connector,
-_dsi_connector_helper_funcs);
- ret = drm_connector_init(bridge->dev, >connector,
-_dsi_connector_funcs,
-DRM_MODE_CONNECTOR_DSI);
- if (ret) {
- dev_err(dsi->dev, "Couldn't initialise the DSI
connector\n");
- goto err_cleanup_connector;
- }
-
- drm_connector_attach_encoder(>connector, >encoder);
- }
-
- return 0;
-err_cleanup_connector:
- drm_encoder_cleanup(>encoder);
- return ret;
+ return drm_bridge_attach(bridge->encoder, dsi->panel_bridge,
+>bridge, flags);
}
static const struct drm_bridge_funcs
