Hi Maxime,
On Mon, Nov 22, 2021 at 3:37 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> On Mon, Nov 22, 2021 at 12:22:19PM +0530, Jagan Teki wrote:
> > Some display panels would come up with a non-DSI output, those
> > can have an option to connect the DSI host by means of interface
> > bridge converter.
> >
> > This DSI to non-DSI interface bridge converter would requires
> > DSI Host to handle drm bridge functionalities in order to DSI
> > Host to Interface bridge.
>
> In order to do this you would need to use the DRM bridge API...
Sorry, which bridge API do you mean?
>
> > This patch convert the existing to a drm bridge driver with a
> > built-in encoder support for compatibility with existing
> > component drivers.
>
> ... but changing the encoder driver to a bridge is completely
> unnecessary to do so. Why did you need to make that change?
Idea of this series is to convert the driver to bridge and use the
latest bridge function from the v1 series.
>
> > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> >
> > ---
> > 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 | 112 ++++++++++++++++++++-----
> > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 ++
> > 2 files changed, 96 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 43d9c9e5198d..a6a272b55f77 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -21,6 +21,7 @@
> >
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_of.h>
> > #include <drm/drm_panel.h>
> > #include <drm/drm_print.h>
> > #include <drm/drm_probe_helper.h>
> > @@ -713,10 +714,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_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
> > - struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> > + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
> > + struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode;
> > struct mipi_dsi_device *device = dsi->device;
> > union phy_configure_opts opts = { };
> > struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
> > @@ -772,6 +774,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> > if (dsi->panel)
> > drm_panel_prepare(dsi->panel);
> >
> > + if (dsi->next_bridge)
> > + dsi->next_bridge->funcs->atomic_pre_enable(dsi->next_bridge, old_bridge_state);
> > +
>
> Please use the proper helpers.
If we use bridge_functions we need to take atomic functions as
precedence as the next bridge functions might convert atomic calls.
>
> > /*
> > * FIXME: This should be moved after the switch to HS mode.
> > *
> > @@ -787,6 +792,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder)
> > if (dsi->panel)
> > drm_panel_enable(dsi->panel);
> >
> > + if (dsi->next_bridge)
> > + dsi->next_bridge->funcs->atomic_enable(dsi->next_bridge, old_bridge_state);
> > +
>
> Ditto
>
> > sun6i_dsi_start(dsi, DSI_START_HSC);
> >
> > udelay(1000);
> > @@ -794,15 +802,19 @@ 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_atomic_disable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_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);
> > + } else if (dsi->next_bridge) {
> > + dsi->next_bridge->funcs->atomic_disable(dsi->next_bridge, old_bridge_state);
> > + dsi->next_bridge->funcs->atomic_post_disable(dsi->next_bridge, old_bridge_state);
>
> Ditto
>
> > }
> >
> > phy_power_off(dsi->dphy);
> > @@ -842,9 +854,25 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
> > .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 int sun6i_dsi_bridge_attach(struct drm_bridge *bridge,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
> > +
> > + if (dsi->next_bridge)
> > + return drm_bridge_attach(bridge->encoder, dsi->next_bridge,
> > + NULL, 0);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct drm_bridge_funcs sun6i_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_enable = sun6i_dsi_bridge_atomic_enable,
> > + .atomic_disable = sun6i_dsi_bridge_atomic_disable,
> > + .attach = sun6i_dsi_bridge_attach,
> > };
> >
> > static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
> > @@ -966,8 +994,6 @@ 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);
> > ret = drm_simple_encoder_init(drm, &dsi->encoder,
> > DRM_MODE_ENCODER_DSI);
> > if (ret) {
> > @@ -976,18 +1002,26 @@ 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");
> > + dev_err(dsi->dev, "Couldn't attach drm bridge\n");
> > goto err_cleanup_connector;
> > }
> >
> > - drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> > + if (dsi->panel) {
> > + 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);
> > + if (ret) {
> > + dev_err(dsi->dev,
> > + "Couldn't initialise the DSI connector\n");
> > + goto err_cleanup_connector;
> > + }
> > +
> > + drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
> > + }
> >
> > return 0;
> >
> > @@ -1013,16 +1047,46 @@ 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);
> > + struct device_node *remote = device->dev.of_node;
> > int ret;
> >
> > - if (IS_ERR(panel))
> > - return PTR_ERR(panel);
> > + if (!of_device_is_available(remote)) {
> > + /**
> > + * I2C interfaced DSI bridges will register DSI host on the
> > + * bridge drivers instead of conventional device.
> > + *
> > + * Those are probed via host of_node instead of device of_node.
> > + */
>
> I have no idea what you mean here. Can you expand on what issue you've
> tried to solve here?
I2C interface DSI bridges will register DSI host on the bridge
drivers. Those can be found using host->dev->of_node and cannot be
find using device->dev.of_node
>
> > + remote = of_graph_get_remote_node(host->dev->of_node, 0, 0);
> > + if (!remote)
> > + return -ENODEV;
> > + }
> > +
> > + dsi->panel = of_drm_find_panel(remote);
> > + if (IS_ERR(dsi->panel)) {
> > + dsi->panel = NULL;
> > +
> > + dsi->next_bridge = of_drm_find_bridge(remote);
> > + if (IS_ERR(dsi->next_bridge)) {
> > + dev_err(dsi->dev, "failed to find bridge\n");
> > + return PTR_ERR(dsi->next_bridge);
> > + }
> > + } else {
> > + dsi->next_bridge = NULL;
> > + }
> > +
> > + of_node_put(remote);
>
> Using devm_drm_of_get_bridge would greatly simplify the driver
I'm aware of this and this would break the existing sunxi dsi binding,
we are not using ports based pipeline in dsi node. Of-course you have
pointed the same before, please check below
https://patchwork.kernel.org/project/dri-devel/patch/20210322140152.101709-2-jagan@xxxxxxxxxxxxxxxxxxxx/
Jagan.
