| Message ID | 20200417175238.27154-4-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support DRM bridges on NVIDIA Tegra | expand |
Hi Dmitry, Thank you for the patch. On Fri, Apr 17, 2020 at 08:52:38PM +0300, Dmitry Osipenko wrote: > Newer Tegra device-trees will specify a video output graph, which involves > LVDS encoder bridge. This patch adds support for the LVDS encoder bridge > to the RGB output, allowing us to model the display hardware properly. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------ > 1 file changed, 49 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c > index 0562a7eb793f..f28a226914c0 100644 > --- a/drivers/gpu/drm/tegra/rgb.c > +++ b/drivers/gpu/drm/tegra/rgb.c > @@ -7,6 +7,7 @@ > #include <linux/clk.h> > > #include <drm/drm_atomic_helper.h> > +#include <drm/drm_bridge_connector.h> > #include <drm/drm_panel.h> > #include <drm/drm_simple_kms_helper.h> > > @@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc) > int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc) > { > struct tegra_output *output = dc->rgb; > + struct drm_connector *connector; > int err; > > if (!dc->rgb) > return -ENODEV; > > - drm_connector_init(drm, &output->connector, &tegra_rgb_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - drm_connector_helper_add(&output->connector, > - &tegra_rgb_connector_helper_funcs); > - output->connector.dpms = DRM_MODE_DPMS_OFF; > - > drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS); > drm_encoder_helper_add(&output->encoder, > &tegra_rgb_encoder_helper_funcs); > > - drm_connector_attach_encoder(&output->connector, > - &output->encoder); > - drm_connector_register(&output->connector); > + /* > + * Tegra devices that have LVDS panel utilize LVDS encoder bridge > + * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that > + * go to display panel's receiver. > + * > + * Encoder usually have a power-down control which needs to be enabled > + * in order to transmit 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 utilize LVDS encoder bridge, which provides > + * us with a connector and handles the display panel. > + * > + * For older device-trees we fall back to our own connector and use > + * nvidia,panel phandle. As I tried to explain before, if you wrap the panel in a bridge with drm_panel_bridge_add() (or the devm_ variant), you will always have a bridge associated with the output, and will be able to remove your custom connector implementation. I thus recommend converting to drm_panel_bridge_add() either as part of this patch series, or just after it, to get full benefits. With the assumption that this will be handled, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + if (output->bridge) { > + err = drm_bridge_attach(&output->encoder, output->bridge, > + NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); > + if (err) { > + dev_err(output->dev, "failed to attach bridge: %d\n", > + err); > + return err; > + } > + > + connector = drm_bridge_connector_init(drm, &output->encoder); > + if (IS_ERR(connector)) { > + dev_err(output->dev, > + "failed to initialize bridge connector: %pe\n", > + connector); > + return PTR_ERR(connector); > + } > + > + drm_connector_attach_encoder(connector, &output->encoder); > + } else { > + drm_connector_init(drm, &output->connector, > + &tegra_rgb_connector_funcs, > + DRM_MODE_CONNECTOR_LVDS); > + drm_connector_helper_add(&output->connector, > + &tegra_rgb_connector_helper_funcs); > + output->connector.dpms = DRM_MODE_DPMS_OFF; > + > + drm_connector_attach_encoder(&output->connector, > + &output->encoder); > + drm_connector_register(&output->connector); > + } > > err = tegra_output_init(drm, output); > if (err < 0) {
17.04.2020 22:24, Laurent Pinchart пишет: ... > As I tried to explain before, if you wrap the panel in a bridge with > drm_panel_bridge_add() (or the devm_ variant), you will always have a > bridge associated with the output, and will be able to remove your > custom connector implementation. I thus recommend converting to > drm_panel_bridge_add() either as part of this patch series, or just > after it, to get full benefits. > > With the assumption that this will be handled, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks you very much! Yes, I got yours point about wrapping panel into the bridge. But I don't think that it's worth the effort right now because each Tegra output has it's own implantation of the connector and it should be cleaner not to touch that code. Secondly, I don't have hardware to test all available panel output types on Tegra and the benefits of messing with all that code are a bit dim to me. I can make a patch to wrap the RGB panel into a bridge, but this should make code a bit inconsistent in regards to not having a common code path for the "legacy" nvidia,panel. So perhaps it's better to leave it all as-is for now.
On Fri, Apr 17, 2020 at 11:11:06PM +0300, Dmitry Osipenko wrote: > 17.04.2020 22:24, Laurent Pinchart пишет: > ... > > As I tried to explain before, if you wrap the panel in a bridge with > > drm_panel_bridge_add() (or the devm_ variant), you will always have a > > bridge associated with the output, and will be able to remove your > > custom connector implementation. I thus recommend converting to > > drm_panel_bridge_add() either as part of this patch series, or just > > after it, to get full benefits. > > > > With the assumption that this will be handled, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks you very much! > > Yes, I got yours point about wrapping panel into the bridge. But I don't > think that it's worth the effort right now because each Tegra output has > it's own implantation of the connector and it should be cleaner not to > touch that code. > > Secondly, I don't have hardware to test all available panel output types > on Tegra and the benefits of messing with all that code are a bit dim to me. > > I can make a patch to wrap the RGB panel into a bridge, but this should > make code a bit inconsistent in regards to not having a common code path > for the "legacy" nvidia,panel. So perhaps it's better to leave it all > as-is for now. I had a brief look at the code, converting the different output types one by one would be a better way forward than not doing anything at all in my opinion :-) Once you convert the first output it will also serve as an example on how to do it, and hopefully other developers with access to hardware could then do more conversions.
17.04.2020 23:34, Laurent Pinchart пишет: > On Fri, Apr 17, 2020 at 11:11:06PM +0300, Dmitry Osipenko wrote: >> 17.04.2020 22:24, Laurent Pinchart пишет: >> ... >>> As I tried to explain before, if you wrap the panel in a bridge with >>> drm_panel_bridge_add() (or the devm_ variant), you will always have a >>> bridge associated with the output, and will be able to remove your >>> custom connector implementation. I thus recommend converting to >>> drm_panel_bridge_add() either as part of this patch series, or just >>> after it, to get full benefits. >>> >>> With the assumption that this will be handled, >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Thanks you very much! >> >> Yes, I got yours point about wrapping panel into the bridge. But I don't >> think that it's worth the effort right now because each Tegra output has >> it's own implantation of the connector and it should be cleaner not to >> touch that code. >> >> Secondly, I don't have hardware to test all available panel output types >> on Tegra and the benefits of messing with all that code are a bit dim to me. >> >> I can make a patch to wrap the RGB panel into a bridge, but this should >> make code a bit inconsistent in regards to not having a common code path >> for the "legacy" nvidia,panel. So perhaps it's better to leave it all >> as-is for now. > > I had a brief look at the code, converting the different output types > one by one would be a better way forward than not doing anything at all > in my opinion :-) Once you convert the first output it will also serve > as an example on how to do it, and hopefully other developers with > access to hardware could then do more conversions. > Thierry, would you want to have RGB panel converted into a bridge? If yes, I'll make a v5 with that change.
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c index 0562a7eb793f..f28a226914c0 100644 --- a/drivers/gpu/drm/tegra/rgb.c +++ b/drivers/gpu/drm/tegra/rgb.c @@ -7,6 +7,7 @@ #include <linux/clk.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_panel.h> #include <drm/drm_simple_kms_helper.h> @@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc) int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc) { struct tegra_output *output = dc->rgb; + struct drm_connector *connector; int err; if (!dc->rgb) return -ENODEV; - drm_connector_init(drm, &output->connector, &tegra_rgb_connector_funcs, - DRM_MODE_CONNECTOR_LVDS); - drm_connector_helper_add(&output->connector, - &tegra_rgb_connector_helper_funcs); - output->connector.dpms = DRM_MODE_DPMS_OFF; - drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS); drm_encoder_helper_add(&output->encoder, &tegra_rgb_encoder_helper_funcs); - drm_connector_attach_encoder(&output->connector, - &output->encoder); - drm_connector_register(&output->connector); + /* + * Tegra devices that have LVDS panel utilize LVDS encoder bridge + * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that + * go to display panel's receiver. + * + * Encoder usually have a power-down control which needs to be enabled + * in order to transmit 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 utilize LVDS encoder bridge, which provides + * us with a connector and handles the display panel. + * + * For older device-trees we fall back to our own connector and use + * nvidia,panel phandle. + */ + if (output->bridge) { + err = drm_bridge_attach(&output->encoder, output->bridge, + NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (err) { + dev_err(output->dev, "failed to attach bridge: %d\n", + err); + return err; + } + + connector = drm_bridge_connector_init(drm, &output->encoder); + if (IS_ERR(connector)) { + dev_err(output->dev, + "failed to initialize bridge connector: %pe\n", + connector); + return PTR_ERR(connector); + } + + drm_connector_attach_encoder(connector, &output->encoder); + } else { + drm_connector_init(drm, &output->connector, + &tegra_rgb_connector_funcs, + DRM_MODE_CONNECTOR_LVDS); + drm_connector_helper_add(&output->connector, + &tegra_rgb_connector_helper_funcs); + output->connector.dpms = DRM_MODE_DPMS_OFF; + + drm_connector_attach_encoder(&output->connector, + &output->encoder); + drm_connector_register(&output->connector); + } err = tegra_output_init(drm, output); if (err < 0) {
Newer Tegra device-trees will specify a video output graph, which involves LVDS encoder bridge. This patch adds support for the LVDS encoder bridge to the RGB output, allowing us to model the display hardware properly. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 9 deletions(-)