| Message ID | 20200128135514.108171-9-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Boris, Thank you for the patch. On Tue, Jan 28, 2020 at 02:55:10PM +0100, Boris Brezillon wrote: > Some DPI -> LVDS encoders might support several input bus width. Add a > DT property to describe the bus width used on a specific design. > > v10: > * Add changelog to the commit message > > v8 -> v9: > * No changes > > v7: > * Fix a use-after-release problem > * Get rid of the data-mapping parsing > * Use kmalloc instead of kcalloc. > > v4 -> v6: > * Not part of the series > > v3: > * Use bus-width for the rgb24/rgb18 distinction > * Adjust code to match core changes > * Get rid of of_get_data_mapping() > * Don't implement ->atomic_check() (the core now takes care of bus > flags propagation) > > v2: > * Make the bus-format negotiation logic more generic > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/gpu/drm/bridge/lvds-codec.c | 64 ++++++++++++++++++++++++++--- > 1 file changed, 58 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c > index 5f04cc11227e..f4fd8472c335 100644 > --- a/drivers/gpu/drm/bridge/lvds-codec.c > +++ b/drivers/gpu/drm/bridge/lvds-codec.c > @@ -11,6 +11,7 @@ > #include <linux/of_graph.h> > #include <linux/platform_device.h> > > +#include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > #include <drm/drm_panel.h> > > @@ -19,6 +20,7 @@ struct lvds_codec { > struct drm_bridge *panel_bridge; > struct gpio_desc *powerdown_gpio; > u32 connector_type; > + u32 input_fmt; > }; > > static int lvds_codec_attach(struct drm_bridge *bridge) > @@ -48,18 +50,47 @@ static void lvds_codec_disable(struct drm_bridge *bridge) > gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1); > } > > +static u32 * > +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + struct lvds_codec *lvds_codec = container_of(bridge, > + struct lvds_codec, bridge); > + u32 *input_fmts; > + > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) { > + *num_input_fmts = 0; > + return NULL; > + } > + > + *num_input_fmts = 1; > + input_fmts[0] = lvds_codec->input_fmt; > + return input_fmts; > +} > + > static struct drm_bridge_funcs funcs = { > .attach = lvds_codec_attach, > .enable = lvds_codec_enable, > .disable = lvds_codec_disable, > + .atomic_reset = drm_atomic_helper_bridge_reset, > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts, > }; > > static int lvds_codec_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device_node *panel_node; > + struct device_node *np; > struct drm_panel *panel; > struct lvds_codec *lvds_codec; > + u32 input_bus_width; > + int err; > > lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); > if (!lvds_codec) > @@ -69,22 +100,43 @@ static int lvds_codec_probe(struct platform_device *pdev) > lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", > GPIOD_OUT_HIGH); > if (IS_ERR(lvds_codec->powerdown_gpio)) { > - int err = PTR_ERR(lvds_codec->powerdown_gpio); > + err = PTR_ERR(lvds_codec->powerdown_gpio); > > if (err != -EPROBE_DEFER) > dev_err(dev, "powerdown GPIO failure: %d\n", err); > return err; > } > > + np = of_graph_get_port_by_id(dev->of_node, 0); > + if (!np) { > + dev_dbg(dev, "port 0 not found\n"); > + return -ENXIO; > + } > + > + err = of_property_read_u32(np, "bus-width", &input_bus_width); > + of_node_put(np); > + > + if (err) { > + lvds_codec->input_fmt = MEDIA_BUS_FMT_FIXED; > + } else if (input_bus_width == 18) { > + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB666_1X18; > + } else if (input_bus_width == 24) { > + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB888_1X24; > + } else { > + dev_dbg(dev, "unsupported bus-width value %u on port 0\n", > + input_bus_width); > + return -ENOTSUPP; ENOTSUPP is "Operation not supported", I'd go for -EINVAL. > + } Doesn't this apply to LVDS encoders only ? For LVDS decoders I don't think we want to report an RGB format on the input. > + > /* Locate the panel DT node. */ > - panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); > - if (!panel_node) { > + np = of_graph_get_remote_node(dev->of_node, 1, 0); > + if (!np) { > dev_dbg(dev, "panel DT node not found\n"); > return -ENXIO; > } > > - panel = of_drm_find_panel(panel_node); > - of_node_put(panel_node); > + panel = of_drm_find_panel(np); > + of_node_put(np); > if (IS_ERR(panel)) { > dev_dbg(dev, "panel not found, deferring probe\n"); > return PTR_ERR(panel);
Hi Boris/Laurent. > > + > > + err = of_property_read_u32(np, "bus-width", &input_bus_width); > > + of_node_put(np); > > + > > + if (err) { > > + lvds_codec->input_fmt = MEDIA_BUS_FMT_FIXED; > > + } else if (input_bus_width == 18) { > > + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB666_1X18; > > + } else if (input_bus_width == 24) { > > + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB888_1X24; > > + } else { > > + dev_dbg(dev, "unsupported bus-width value %u on port 0\n", > > + input_bus_width); > > + return -ENOTSUPP; > > ENOTSUPP is "Operation not supported", I'd go for -EINVAL. > > > + } > > Doesn't this apply to LVDS encoders only ? For LVDS decoders I don't > think we want to report an RGB format on the input. In panel-lvds we use the property "data-mapping" for the same purpose. To specify the MEDIA_BUS format. It would be good to standardize on the same property, and maybe have the same binding descriptions for all. And "data-mapping" is a text string, which gives us more flexibility than just a number, that for MEDIA_BUS_FMT seems required. Sam
On Tue, 25 Feb 2020 07:15:43 +0100 Sam Ravnborg <sam@ravnborg.org> wrote: > Hi Boris/Laurent. > > > > + > > > + err = of_property_read_u32(np, "bus-width", &input_bus_width); > > > + of_node_put(np); > > > + > > > + if (err) { > > > + lvds_codec->input_fmt = MEDIA_BUS_FMT_FIXED; > > > + } else if (input_bus_width == 18) { > > > + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB666_1X18; > > > + } else if (input_bus_width == 24) { > > > + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB888_1X24; > > > + } else { > > > + dev_dbg(dev, "unsupported bus-width value %u on port 0\n", > > > + input_bus_width); > > > + return -ENOTSUPP; > > > > ENOTSUPP is "Operation not supported", I'd go for -EINVAL. > > > > > + } > > > > Doesn't this apply to LVDS encoders only ? For LVDS decoders I don't > > think we want to report an RGB format on the input. > > In panel-lvds we use the property "data-mapping" for the same purpose. > To specify the MEDIA_BUS format. I started with data-mapping, and was told (by Laurent IIRC) that bus-width would be more appropriate for a DPI (AKA RGB) bus. I think it has to do with the fact that fully-parallel buses always have one color bit per-signal, while serial or partially-parallel buses can have several color-bits per-signal, the assignment being described by this 'data-mapping' property. This being said, I can see a case where data-mapping would be needed for DPI buses => RGB component ordering. A 24bit bus does not distinguish between RGB888 and BGR888. > > It would be good to standardize on the same property, and maybe have the > same binding descriptions for all. As for the standardization, I'm all for it, but let's do that in a second step, please.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 5f04cc11227e..f4fd8472c335 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -11,6 +11,7 @@ #include <linux/of_graph.h> #include <linux/platform_device.h> +#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_panel.h> @@ -19,6 +20,7 @@ struct lvds_codec { struct drm_bridge *panel_bridge; struct gpio_desc *powerdown_gpio; u32 connector_type; + u32 input_fmt; }; static int lvds_codec_attach(struct drm_bridge *bridge) @@ -48,18 +50,47 @@ static void lvds_codec_disable(struct drm_bridge *bridge) gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1); } +static u32 * +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + struct lvds_codec *lvds_codec = container_of(bridge, + struct lvds_codec, bridge); + u32 *input_fmts; + + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) { + *num_input_fmts = 0; + return NULL; + } + + *num_input_fmts = 1; + input_fmts[0] = lvds_codec->input_fmt; + return input_fmts; +} + static struct drm_bridge_funcs funcs = { .attach = lvds_codec_attach, .enable = lvds_codec_enable, .disable = lvds_codec_disable, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts, }; static int lvds_codec_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *panel_node; + struct device_node *np; struct drm_panel *panel; struct lvds_codec *lvds_codec; + u32 input_bus_width; + int err; lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); if (!lvds_codec) @@ -69,22 +100,43 @@ static int lvds_codec_probe(struct platform_device *pdev) lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); if (IS_ERR(lvds_codec->powerdown_gpio)) { - int err = PTR_ERR(lvds_codec->powerdown_gpio); + err = PTR_ERR(lvds_codec->powerdown_gpio); if (err != -EPROBE_DEFER) dev_err(dev, "powerdown GPIO failure: %d\n", err); return err; } + np = of_graph_get_port_by_id(dev->of_node, 0); + if (!np) { + dev_dbg(dev, "port 0 not found\n"); + return -ENXIO; + } + + err = of_property_read_u32(np, "bus-width", &input_bus_width); + of_node_put(np); + + if (err) { + lvds_codec->input_fmt = MEDIA_BUS_FMT_FIXED; + } else if (input_bus_width == 18) { + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB666_1X18; + } else if (input_bus_width == 24) { + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB888_1X24; + } else { + dev_dbg(dev, "unsupported bus-width value %u on port 0\n", + input_bus_width); + return -ENOTSUPP; + } + /* Locate the panel DT node. */ - panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); - if (!panel_node) { + np = of_graph_get_remote_node(dev->of_node, 1, 0); + if (!np) { dev_dbg(dev, "panel DT node not found\n"); return -ENXIO; } - panel = of_drm_find_panel(panel_node); - of_node_put(panel_node); + panel = of_drm_find_panel(np); + of_node_put(np); if (IS_ERR(panel)) { dev_dbg(dev, "panel not found, deferring probe\n"); return PTR_ERR(panel);
Some DPI -> LVDS encoders might support several input bus width. Add a DT property to describe the bus width used on a specific design. v10: * Add changelog to the commit message v8 -> v9: * No changes v7: * Fix a use-after-release problem * Get rid of the data-mapping parsing * Use kmalloc instead of kcalloc. v4 -> v6: * Not part of the series v3: * Use bus-width for the rgb24/rgb18 distinction * Adjust code to match core changes * Get rid of of_get_data_mapping() * Don't implement ->atomic_check() (the core now takes care of bus flags propagation) v2: * Make the bus-format negotiation logic more generic Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/gpu/drm/bridge/lvds-codec.c | 64 ++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 6 deletions(-)