| Message ID | 20200416172405.5051-3-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support DRM bridges on NVIDIA Tegra | expand |
Hi Dmitry, Thank you for the patch. On Thu, Apr 16, 2020 at 08:24:05PM +0300, Dmitry Osipenko wrote: > Newer Tegra device-trees will specify a video output graph that involves > LVDS encoder bridge, This patch adds support for the LVDS encoder bridge > to the RGB output, allowing us to model display hardware properly. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/drm/tegra/drm.h | 2 ++ > drivers/gpu/drm/tegra/output.c | 10 ++++++++++ > drivers/gpu/drm/tegra/rgb.c | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > > diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h > index 804869799305..cccd368b6752 100644 > --- a/drivers/gpu/drm/tegra/drm.h > +++ b/drivers/gpu/drm/tegra/drm.h > @@ -12,6 +12,7 @@ > #include <linux/of_gpio.h> > > #include <drm/drm_atomic.h> > +#include <drm/drm_bridge.h> > #include <drm/drm_edid.h> > #include <drm/drm_encoder.h> > #include <drm/drm_fb_helper.h> > @@ -116,6 +117,7 @@ struct tegra_output { > struct device_node *of_node; > struct device *dev; > > + struct drm_bridge *bridge; > struct drm_panel *panel; > struct i2c_adapter *ddc; > const struct edid *edid; > diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c > index a6a711d54e88..37fc6b8c173f 100644 > --- a/drivers/gpu/drm/tegra/output.c > +++ b/drivers/gpu/drm/tegra/output.c > @@ -180,6 +180,16 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output) > int connector_type; > int err; > > + if (output->bridge) { > + err = drm_bridge_attach(&output->encoder, output->bridge, > + NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); Using DRM_BRIDGE_ATTACH_NO_CONNECTOR is definitely the way to go, but please be aware that it will require creating a connector and an encoder manually (which I think this driver already does), and using the bridge operations to implement the connector operations. I highly recommend using the DRM bridge connector helper for this purpose. > + if (err) { > + dev_err(output->dev, "cannot connect bridge: %d\n", > + err); > + return err; > + } > + } > + > if (output->panel) { May I also recommend switching to the DRM panel bridge helper ? It will simplify the code. > err = drm_panel_attach(output->panel, &output->connector); > if (err < 0) > diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c > index 0562a7eb793f..0df213e92664 100644 > --- a/drivers/gpu/drm/tegra/rgb.c > +++ b/drivers/gpu/drm/tegra/rgb.c > @@ -5,6 +5,7 @@ > */ > > #include <linux/clk.h> > +#include <linux/of_graph.h> > > #include <drm/drm_atomic_helper.h> > #include <drm/drm_panel.h> > @@ -210,6 +211,7 @@ static const struct drm_encoder_helper_funcs tegra_rgb_encoder_helper_funcs = { > > int tegra_dc_rgb_probe(struct tegra_dc *dc) > { > + const unsigned int encoder_port = 0, panel_port = 1; > struct device_node *np; > struct tegra_rgb *rgb; > int err; > @@ -226,6 +228,38 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc) > rgb->output.of_node = np; > rgb->dc = dc; > > + /* > + * Tegra devices that have LVDS panel utilize LVDS-encoder bridge > + * for converting 24/18 RGB data-lanes into 8 lanes. Encoder usually > + * have a powerdown control which needs to be enabled in order to > + * transfer data to the panel. Historically devices that use an older > + * device-tree version didn't model the bridge, assuming that encoder > + * is turned ON by default, while today's DRM allows us to model LVDS > + * encoder properly. > + * > + * Newer device-trees may utilize output->encoder->panel graph. > + * > + * For older device-trees we fall back to use nvidia,panel phandle. > + */ > + np = of_graph_get_remote_node(rgb->output.of_node, encoder_port, -1); > + if (np) { > + rgb->output.bridge = of_drm_find_bridge(np); > + of_node_put(np); > + > + if (!rgb->output.bridge) > + return -EPROBE_DEFER; > + > + np = of_graph_get_remote_node(rgb->output.bridge->of_node, > + panel_port, -1); This shouldn't be needed, the LVDS codec bridge driver should already get the panel and handle it internally. You only need to handle panels in this driver when they're connected directly to the RGB input. > + if (np) { > + rgb->output.panel = of_drm_find_panel(np); > + of_node_put(np); > + > + if (IS_ERR(rgb->output.panel)) > + return PTR_ERR(rgb->output.panel); > + } > + } > + > err = tegra_output_probe(&rgb->output); > if (err < 0) > return err;
16.04.2020 20:41, Laurent Pinchart пишет: ... >> + if (output->bridge) { >> + err = drm_bridge_attach(&output->encoder, output->bridge, >> + NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > Using DRM_BRIDGE_ATTACH_NO_CONNECTOR is definitely the way to go, but > please be aware that it will require creating a connector and an encoder > manually (which I think this driver already does), and using the bridge > operations to implement the connector operations. I highly recommend > using the DRM bridge connector helper for this purpose. Okay, I missed that there is a DRM bridge connector helper. Thank you very much for the suggestion, I'll switch to the bridge helper in v4. >> + if (err) { >> + dev_err(output->dev, "cannot connect bridge: %d\n", >> + err); >> + return err; >> + } >> + } >> + >> if (output->panel) { > > May I also recommend switching to the DRM panel bridge helper ? It will > simplify the code. Could you please clarify what is the "DRM panel bridge helper"? I think we won't need any additional helpers after switching to the bridge connector helper, no? ... >> + np = of_graph_get_remote_node(rgb->output.bridge->of_node, >> + panel_port, -1); > > This shouldn't be needed, the LVDS codec bridge driver should already > get the panel and handle it internally. You only need to handle panels > in this driver when they're connected directly to the RGB input. Indeed, it won't be needed if we will use the bridge connector helper. Thank you very much for the clarifications!
16.04.2020 21:52, Dmitry Osipenko пишет: ... >> May I also recommend switching to the DRM panel bridge helper ? It will >> simplify the code. > > Could you please clarify what is the "DRM panel bridge helper"? > > I think we won't need any additional helpers after switching to the > bridge connector helper, no? Actually, I now see that the panel needs to be manually attached to the connector. Still it's not apparent to me how to get panel out of the bridge. It looks like there is no such "panel helper" for the bridge API or I just can't find it.
Hi Dimitry. On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote: > 16.04.2020 21:52, Dmitry Osipenko пишет: > ... > >> May I also recommend switching to the DRM panel bridge helper ? It will > >> simplify the code. > > > > Could you please clarify what is the "DRM panel bridge helper"? > > > > I think we won't need any additional helpers after switching to the > > bridge connector helper, no? > > Actually, I now see that the panel needs to be manually attached to the > connector. > > Still it's not apparent to me how to get panel out of the bridge. It > looks like there is no such "panel helper" for the bridge API or I just > can't find it. Take a look in bridge/panel.c I think devm_drm_panel_bridge_add() is what you are after. Sam
Hi Dmitry, On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote: > 16.04.2020 21:52, Dmitry Osipenko пишет: > ... > >> May I also recommend switching to the DRM panel bridge helper ? It will > >> simplify the code. > > > > Could you please clarify what is the "DRM panel bridge helper"? > > > > I think we won't need any additional helpers after switching to the > > bridge connector helper, no? > > Actually, I now see that the panel needs to be manually attached to the > connector. The DRM panel bridge helper creates a bridge from a panel (with devm_drm_panel_bridge_add()). You can then attach that bridge to the chain, like any other bridge, and the enable/disable operations will be called automatically without any need to call the panel enable/disable manually as done currently. > Still it's not apparent to me how to get panel out of the bridge. It > looks like there is no such "panel helper" for the bridge API or I just > can't find it. You don't need to get a panel out of the bridge. You should get the bridge as done today, and wrap it in a bridge with devm_drm_panel_bridge_add().
16.04.2020 23:50, Laurent Pinchart пишет: > Hi Dmitry, > > On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote: >> 16.04.2020 21:52, Dmitry Osipenko пишет: >> ... >>>> May I also recommend switching to the DRM panel bridge helper ? It will >>>> simplify the code. >>> >>> Could you please clarify what is the "DRM panel bridge helper"? >>> >>> I think we won't need any additional helpers after switching to the >>> bridge connector helper, no? >> >> Actually, I now see that the panel needs to be manually attached to the >> connector. > > The DRM panel bridge helper creates a bridge from a panel (with > devm_drm_panel_bridge_add()). You can then attach that bridge to the > chain, like any other bridge, and the enable/disable operations will be > called automatically without any need to call the panel enable/disable > manually as done currently. > >> Still it's not apparent to me how to get panel out of the bridge. It >> looks like there is no such "panel helper" for the bridge API or I just >> can't find it. > > You don't need to get a panel out of the bridge. You should get the > bridge as done today, You mean "get the panel", correct? > and wrap it in a bridge with > devm_drm_panel_bridge_add(). > The lvds-codec already wraps panel into the bridge using devm_drm_panel_bridge_add() and chains it for us, please see lvds_codec_probe() / lvds_codec_attach(). Does it mean that lvds-codec is not supporting the new model? Everything works nicely using the old model, where bridge creates connector and attaches panel to it for us. [I'm still not sure what is the point to use the new model in a case of a simple chain of bridges] Using the new model, the connector isn't created by the bridge, so I need to duplicate that creation effort in the driver. Once the bridge connector is manually created, I need to attach panel to this connector, but panel is reachable only via the remote bridge (which wraps the panel). driver connector -> LVDS bridge -> panel bridge -> panel
Hi Dmitry, On Fri, Apr 17, 2020 at 12:15:33AM +0300, Dmitry Osipenko wrote: > 16.04.2020 23:50, Laurent Pinchart пишет: > > On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote: > >> 16.04.2020 21:52, Dmitry Osipenko пишет: > >> ... > >>>> May I also recommend switching to the DRM panel bridge helper ? It will > >>>> simplify the code. > >>> > >>> Could you please clarify what is the "DRM panel bridge helper"? > >>> > >>> I think we won't need any additional helpers after switching to the > >>> bridge connector helper, no? > >> > >> Actually, I now see that the panel needs to be manually attached to the > >> connector. > > > > The DRM panel bridge helper creates a bridge from a panel (with > > devm_drm_panel_bridge_add()). You can then attach that bridge to the > > chain, like any other bridge, and the enable/disable operations will be > > called automatically without any need to call the panel enable/disable > > manually as done currently. > > > >> Still it's not apparent to me how to get panel out of the bridge. It > >> looks like there is no such "panel helper" for the bridge API or I just > >> can't find it. > > > > You don't need to get a panel out of the bridge. You should get the > > bridge as done today, > > You mean "get the panel", correct? Yes, sorry. > > and wrap it in a bridge with > > devm_drm_panel_bridge_add(). > > > > The lvds-codec already wraps panel into the bridge using > devm_drm_panel_bridge_add() and chains it for us, please see > lvds_codec_probe() / lvds_codec_attach(). > > Does it mean that lvds-codec is not supporting the new model? No, that *is* the new model :-) As I think I've commented during review, when you have an LVDS encoder and a panel, you don't need to handle the panel at all. When you have an RGB panel connected directly to the RGB output, you need to wrap it in a bridge, exactly the same way as lvds-codec does for its panel. > Everything works nicely using the old model, where bridge creates > connector and attaches panel to it for us. > > [I'm still not sure what is the point to use the new model in a case of > a simple chain of bridges] > > Using the new model, the connector isn't created by the bridge, so I > need to duplicate that creation effort in the driver. Once the bridge > connector is manually created, I need to attach panel to this connector, > but panel is reachable only via the remote bridge (which wraps the panel). > > driver connector -> LVDS bridge -> panel bridge -> panel With the new model, 1. The display driver and the bridge drivers need to get hold of the bridge directly connected to their output (for instance with of_drm_find_panel()). If the output is connected to a panel, they need to wrap that panel in a bridge (with devm_drm_panel_bridge_add()). I plan, in the future, to make creation of panel bridges automatic, so drivers won't have to care. 2. The display driver needs to create a dummy drm_encoder for each of its outputs (for instance with drm_simple_encoder_init()). 3. The display driver needs to create a drm_connector for each of its outputs, and implement connector operations by delegating them to the bridges in the pipeline. Unless there's a good reason not to do so, this should be done with drm_bridge_connector_init(). That's it. Every driver then focusses on its own needs, bridge drivers handle only the device they're associated with, and the DRM core and DRM bridge connector helper will handle all the rest.
17.04.2020 00:39, Laurent Pinchart пишет: > Hi Dmitry, > > On Fri, Apr 17, 2020 at 12:15:33AM +0300, Dmitry Osipenko wrote: >> 16.04.2020 23:50, Laurent Pinchart пишет: >>> On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote: >>>> 16.04.2020 21:52, Dmitry Osipenko пишет: >>>> ... >>>>>> May I also recommend switching to the DRM panel bridge helper ? It will >>>>>> simplify the code. >>>>> >>>>> Could you please clarify what is the "DRM panel bridge helper"? >>>>> >>>>> I think we won't need any additional helpers after switching to the >>>>> bridge connector helper, no? >>>> >>>> Actually, I now see that the panel needs to be manually attached to the >>>> connector. >>> >>> The DRM panel bridge helper creates a bridge from a panel (with >>> devm_drm_panel_bridge_add()). You can then attach that bridge to the >>> chain, like any other bridge, and the enable/disable operations will be >>> called automatically without any need to call the panel enable/disable >>> manually as done currently. >>> >>>> Still it's not apparent to me how to get panel out of the bridge. It >>>> looks like there is no such "panel helper" for the bridge API or I just >>>> can't find it. >>> >>> You don't need to get a panel out of the bridge. You should get the >>> bridge as done today, >> >> You mean "get the panel", correct? > > Yes, sorry. > >>> and wrap it in a bridge with >>> devm_drm_panel_bridge_add(). >>> >> >> The lvds-codec already wraps panel into the bridge using >> devm_drm_panel_bridge_add() and chains it for us, please see >> lvds_codec_probe() / lvds_codec_attach(). >> >> Does it mean that lvds-codec is not supporting the new model? > > No, that *is* the new model :-) As I think I've commented during review, > when you have an LVDS encoder and a panel, you don't need to handle the > panel at all. When you have an RGB panel connected directly to the RGB > output, you need to wrap it in a bridge, exactly the same way as > lvds-codec does for its panel. > >> Everything works nicely using the old model, where bridge creates >> connector and attaches panel to it for us. >> >> [I'm still not sure what is the point to use the new model in a case of >> a simple chain of bridges] >> >> Using the new model, the connector isn't created by the bridge, so I >> need to duplicate that creation effort in the driver. Once the bridge >> connector is manually created, I need to attach panel to this connector, >> but panel is reachable only via the remote bridge (which wraps the panel). >> >> driver connector -> LVDS bridge -> panel bridge -> panel > > With the new model, > > 1. The display driver and the bridge drivers need to get hold of the > bridge directly connected to their output (for instance with > of_drm_find_panel()). If the output is connected to a panel, they > need to wrap that panel in a bridge (with > devm_drm_panel_bridge_add()). I plan, in the future, to make creation > of panel bridges automatic, so drivers won't have to care. > > 2. The display driver needs to create a dummy drm_encoder for each of > its outputs (for instance with drm_simple_encoder_init()). > > 3. The display driver needs to create a drm_connector for each of its > outputs, and implement connector operations by delegating them to the > bridges in the pipeline. Unless there's a good reason not to do so, > this should be done with drm_bridge_connector_init(). > > That's it. Every driver then focusses on its own needs, bridge drivers > handle only the device they're associated with, and the DRM core and DRM > bridge connector helper will handle all the rest. > Thank you very much for the clarification again! :) Now I realized what was the missing part.. in my case display panel is mounted upside-down and display output needs to be rotated by 180°. I have a local patch (hack) that adds orientation-property support to the panel-lvds driver and previously the panel's orientation was assigned to the connector using drm_panel_attach(), but now this attachment doesn't happen and I found that panel_lvds_get_modes() missed drm_connector_set_panel_orientation() in my patch. Everything is okay once the panel_lvds_get_modes() is corrected. I'll prepare the v4.
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 804869799305..cccd368b6752 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -12,6 +12,7 @@ #include <linux/of_gpio.h> #include <drm/drm_atomic.h> +#include <drm/drm_bridge.h> #include <drm/drm_edid.h> #include <drm/drm_encoder.h> #include <drm/drm_fb_helper.h> @@ -116,6 +117,7 @@ struct tegra_output { struct device_node *of_node; struct device *dev; + struct drm_bridge *bridge; struct drm_panel *panel; struct i2c_adapter *ddc; const struct edid *edid; diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c index a6a711d54e88..37fc6b8c173f 100644 --- a/drivers/gpu/drm/tegra/output.c +++ b/drivers/gpu/drm/tegra/output.c @@ -180,6 +180,16 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output) int connector_type; int err; + if (output->bridge) { + err = drm_bridge_attach(&output->encoder, output->bridge, + NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (err) { + dev_err(output->dev, "cannot connect bridge: %d\n", + err); + return err; + } + } + if (output->panel) { err = drm_panel_attach(output->panel, &output->connector); if (err < 0) diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c index 0562a7eb793f..0df213e92664 100644 --- a/drivers/gpu/drm/tegra/rgb.c +++ b/drivers/gpu/drm/tegra/rgb.c @@ -5,6 +5,7 @@ */ #include <linux/clk.h> +#include <linux/of_graph.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_panel.h> @@ -210,6 +211,7 @@ static const struct drm_encoder_helper_funcs tegra_rgb_encoder_helper_funcs = { int tegra_dc_rgb_probe(struct tegra_dc *dc) { + const unsigned int encoder_port = 0, panel_port = 1; struct device_node *np; struct tegra_rgb *rgb; int err; @@ -226,6 +228,38 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc) rgb->output.of_node = np; rgb->dc = dc; + /* + * Tegra devices that have LVDS panel utilize LVDS-encoder bridge + * for converting 24/18 RGB data-lanes into 8 lanes. Encoder usually + * have a powerdown control which needs to be enabled in order to + * transfer data to the panel. Historically devices that use an older + * device-tree version didn't model the bridge, assuming that encoder + * is turned ON by default, while today's DRM allows us to model LVDS + * encoder properly. + * + * Newer device-trees may utilize output->encoder->panel graph. + * + * For older device-trees we fall back to use nvidia,panel phandle. + */ + np = of_graph_get_remote_node(rgb->output.of_node, encoder_port, -1); + if (np) { + rgb->output.bridge = of_drm_find_bridge(np); + of_node_put(np); + + if (!rgb->output.bridge) + return -EPROBE_DEFER; + + np = of_graph_get_remote_node(rgb->output.bridge->of_node, + panel_port, -1); + if (np) { + rgb->output.panel = of_drm_find_panel(np); + of_node_put(np); + + if (IS_ERR(rgb->output.panel)) + return PTR_ERR(rgb->output.panel); + } + } + err = tegra_output_probe(&rgb->output); if (err < 0) return err;
Newer Tegra device-trees will specify a video output graph that involves LVDS encoder bridge, This patch adds support for the LVDS encoder bridge to the RGB output, allowing us to model display hardware properly. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpu/drm/tegra/drm.h | 2 ++ drivers/gpu/drm/tegra/output.c | 10 ++++++++++ drivers/gpu/drm/tegra/rgb.c | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+)