| Message ID | 20210515204656.367442-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select | expand |
Hi Marek, Thank you for the patch. On Sat, May 15, 2021 at 10:46:56PM +0200, Marek Vasut wrote: > Decoder input LVDS format is a property of the decoder chip or even > its strapping. Handle data-mapping the same way lvds-panel does. In > case data-mapping is not present, do nothing, since there are still > legacy bindings which do not specify this property. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > To: dri-devel@lists.freedesktop.org > --- > drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index 8a7cb267ab14..33f992d52902 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -23,6 +23,7 @@ struct lvds_codec { > struct regulator *vcc; > struct gpio_desc *powerdown_gpio; > u32 connector_type; > + unsigned int bus_format; > }; > > static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) > @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) > "Failed to disable regulator \"vcc\": %d\n", ret); > } > > +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adj) > +{ > + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); > + struct drm_encoder *encoder = bridge->encoder; > + struct drm_device *ddev = encoder->dev; > + struct drm_connector *connector; > + > + /* If 'data-mapping' was not specified, do nothing. */ > + if (!lvds_codec->bus_format) > + return true; > + > + /* Patch in the LVDS format */ > + list_for_each_entry(connector, &ddev->mode_config.connector_list, head) { > + drm_display_info_set_bus_formats(&connector->display_info, > + &lvds_codec->bus_format, 1); > + } This part bothers me, as the format at the input of the LVDS decoder doesn't match the format on the connector. Shouldn't you implement .atomic_get_output_bus_fmts() instead ? > + > + return true; > +} > + > static const struct drm_bridge_funcs funcs = { > .attach = lvds_codec_attach, > .enable = lvds_codec_enable, > .disable = lvds_codec_disable, > + .mode_fixup = lvds_codec_mode_fixup, > }; > > static int lvds_codec_probe(struct platform_device *pdev) > @@ -81,6 +105,7 @@ static int lvds_codec_probe(struct platform_device *pdev) > struct device_node *panel_node; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > + const char *mapping; I would have moved this variable to the if () { ... } below, but maybe that's just me. > u32 val; > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > @@ -133,6 +158,31 @@ static int lvds_codec_probe(struct platform_device *pdev) > DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; > } > > + /* > + * Decoder input LVDS format is a property of the decoder chip or even > + * its strapping. Handle data-mapping the same way lvds-panel does. In > + * case data-mapping is not present, do nothing, since there are still > + * legacy bindings which do not specify this property. > + */ > + if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) { > + ret = of_property_read_string(dev->of_node, "data-mapping", > + &mapping); > + if (ret < 0) { > + dev_err(dev, "missing 'data-mapping' DT property\n"); > + } else { > + if (!strcmp(mapping, "jeida-18")) { > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG; > + } else if (!strcmp(mapping, "jeida-24")) { > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA; > + } else if (!strcmp(mapping, "vesa-24")) { > + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG; > + } else { > + dev_err(dev, "invalid 'data-mapping' DT property\n"); > + return -EINVAL; > + } > + } > + } > + > /* > * The panel_bridge bridge is attached to the panel's of_node, > * but we need a bridge attached to our of_node for our user
On 5/18/21 1:03 AM, Laurent Pinchart wrote: Hi, [...] >> @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) >> "Failed to disable regulator \"vcc\": %d\n", ret); >> } >> >> +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode, >> + struct drm_display_mode *adj) >> +{ >> + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); >> + struct drm_encoder *encoder = bridge->encoder; >> + struct drm_device *ddev = encoder->dev; >> + struct drm_connector *connector; >> + >> + /* If 'data-mapping' was not specified, do nothing. */ >> + if (!lvds_codec->bus_format) >> + return true; >> + >> + /* Patch in the LVDS format */ >> + list_for_each_entry(connector, &ddev->mode_config.connector_list, head) { >> + drm_display_info_set_bus_formats(&connector->display_info, >> + &lvds_codec->bus_format, 1); >> + } > > This part bothers me, as the format at the input of the LVDS decoder > doesn't match the format on the connector. Shouldn't you implement > .atomic_get_output_bus_fmts() instead ? No, I tried that option before this solution and that didn't work. The atomic stuff seems to be separate from what I need to pass here, i.e. without this, e.g. the mxsfb scanout engine still receives the wrong format. [...]
Hi Marek, On Tue, May 25, 2021 at 12:38:47PM +0200, Marek Vasut wrote: > On 5/18/21 1:03 AM, Laurent Pinchart wrote: > > Hi, > > [...] > > >> @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) > >> "Failed to disable regulator \"vcc\": %d\n", ret); > >> } > >> > >> +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge, > >> + const struct drm_display_mode *mode, > >> + struct drm_display_mode *adj) > >> +{ > >> + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); > >> + struct drm_encoder *encoder = bridge->encoder; > >> + struct drm_device *ddev = encoder->dev; > >> + struct drm_connector *connector; > >> + > >> + /* If 'data-mapping' was not specified, do nothing. */ > >> + if (!lvds_codec->bus_format) > >> + return true; > >> + > >> + /* Patch in the LVDS format */ > >> + list_for_each_entry(connector, &ddev->mode_config.connector_list, head) { > >> + drm_display_info_set_bus_formats(&connector->display_info, > >> + &lvds_codec->bus_format, 1); > >> + } > > > > This part bothers me, as the format at the input of the LVDS decoder > > doesn't match the format on the connector. Shouldn't you implement > > .atomic_get_output_bus_fmts() instead ? > > No, I tried that option before this solution and that didn't work. The > atomic stuff seems to be separate from what I need to pass here, i.e. > without this, e.g. the mxsfb scanout engine still receives the wrong format. I fear this needs to be investigated then, as the connected format isn't the same as the LVDS decoder input format, so the above isn't right.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 8a7cb267ab14..33f992d52902 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -23,6 +23,7 @@ struct lvds_codec { struct regulator *vcc; struct gpio_desc *powerdown_gpio; u32 connector_type; + unsigned int bus_format; }; static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge) @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge) "Failed to disable regulator \"vcc\": %d\n", ret); } +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + struct drm_display_mode *adj) +{ + struct lvds_codec *lvds_codec = to_lvds_codec(bridge); + struct drm_encoder *encoder = bridge->encoder; + struct drm_device *ddev = encoder->dev; + struct drm_connector *connector; + + /* If 'data-mapping' was not specified, do nothing. */ + if (!lvds_codec->bus_format) + return true; + + /* Patch in the LVDS format */ + list_for_each_entry(connector, &ddev->mode_config.connector_list, head) { + drm_display_info_set_bus_formats(&connector->display_info, + &lvds_codec->bus_format, 1); + } + + return true; +} + static const struct drm_bridge_funcs funcs = { .attach = lvds_codec_attach, .enable = lvds_codec_enable, .disable = lvds_codec_disable, + .mode_fixup = lvds_codec_mode_fixup, }; static int lvds_codec_probe(struct platform_device *pdev) @@ -81,6 +105,7 @@ static int lvds_codec_probe(struct platform_device *pdev) struct device_node *panel_node; struct drm_panel *panel; struct lvds_codec *lvds_codec; + const char *mapping; u32 val; lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); @@ -133,6 +158,31 @@ static int lvds_codec_probe(struct platform_device *pdev) DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE; } + /* + * Decoder input LVDS format is a property of the decoder chip or even + * its strapping. Handle data-mapping the same way lvds-panel does. In + * case data-mapping is not present, do nothing, since there are still + * legacy bindings which do not specify this property. + */ + if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) { + ret = of_property_read_string(dev->of_node, "data-mapping", + &mapping); + if (ret < 0) { + dev_err(dev, "missing 'data-mapping' DT property\n"); + } else { + if (!strcmp(mapping, "jeida-18")) { + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG; + } else if (!strcmp(mapping, "jeida-24")) { + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA; + } else if (!strcmp(mapping, "vesa-24")) { + lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG; + } else { + dev_err(dev, "invalid 'data-mapping' DT property\n"); + return -EINVAL; + } + } + } + /* * The panel_bridge bridge is attached to the panel's of_node, * but we need a bridge attached to our of_node for our user
Decoder input LVDS format is a property of the decoder chip or even its strapping. Handle data-mapping the same way lvds-panel does. In case data-mapping is not present, do nothing, since there are still legacy bindings which do not specify this property. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Sam Ravnborg <sam@ravnborg.org> To: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)