| Message ID | 20220113144305.1074389-1-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: sii902x: add support for DRM_BRIDGE_ATTACH_NO_CONNECTOR | expand |
Hey Neil, On Thu, 13 Jan 2022 at 15:43, Neil Armstrong <narmstrong@baylibre.com> wrote: > > This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the > bridge get_edid() and detect() callbacks after refactoring the connector > get_modes() and connector_detect() callbacks. > > In order to keep the bridge working, extra code in get_modes() has been > moved to more logical places. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/sii902x.c | 129 ++++++++++++++++++++++++------- > 1 file changed, 99 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > index 89558e581530..65549fbfdc87 100644 > --- a/drivers/gpu/drm/bridge/sii902x.c > +++ b/drivers/gpu/drm/bridge/sii902x.c > @@ -166,10 +166,12 @@ struct sii902x { > struct i2c_client *i2c; > struct regmap *regmap; > struct drm_bridge bridge; > + struct drm_bridge *next_bridge; > struct drm_connector connector; > struct gpio_desc *reset_gpio; > struct i2c_mux_core *i2cmux; > struct regulator_bulk_data supplies[2]; > + bool sink_is_hdmi; > /* > * Mutex protects audio and video functions from interfering > * each other, by keeping their i2c command sequences atomic. > @@ -245,10 +247,8 @@ static void sii902x_reset(struct sii902x *sii902x) > gpiod_set_value(sii902x->reset_gpio, 0); > } > > -static enum drm_connector_status > -sii902x_connector_detect(struct drm_connector *connector, bool force) > +static enum drm_connector_status sii902x_detect(struct sii902x *sii902x) > { > - struct sii902x *sii902x = connector_to_sii902x(connector); > unsigned int status; > > mutex_lock(&sii902x->mutex); > @@ -261,6 +261,14 @@ sii902x_connector_detect(struct drm_connector *connector, bool force) > connector_status_connected : connector_status_disconnected; > } > > +static enum drm_connector_status > +sii902x_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct sii902x *sii902x = connector_to_sii902x(connector); > + > + return sii902x_detect(sii902x); > +} > + > static const struct drm_connector_funcs sii902x_connector_funcs = { > .detect = sii902x_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > @@ -270,42 +278,40 @@ static const struct drm_connector_funcs sii902x_connector_funcs = { > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > -static int sii902x_get_modes(struct drm_connector *connector) > +static struct edid *sii902x_get_edid(struct sii902x *sii902x, > + struct drm_connector *connector) > { > - struct sii902x *sii902x = connector_to_sii902x(connector); > - u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > - u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; > struct edid *edid; > - int num = 0, ret; > > mutex_lock(&sii902x->mutex); > > edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); > - drm_connector_update_edid_property(connector, edid); > if (edid) { > if (drm_detect_hdmi_monitor(edid)) > - output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; > - > - num = drm_add_edid_modes(connector, edid); > - kfree(edid); > + sii902x->sink_is_hdmi = true; > + else > + sii902x->sink_is_hdmi = false; > } > > - ret = drm_display_info_set_bus_formats(&connector->display_info, > - &bus_format, 1); > - if (ret) > - goto error_out; > + mutex_unlock(&sii902x->mutex); > > - ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, > - SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); > - if (ret) > - goto error_out; > + return edid; > +} > > - ret = num; > +static int sii902x_get_modes(struct drm_connector *connector) > +{ > + struct sii902x *sii902x = connector_to_sii902x(connector); > + struct edid *edid; > + int num = 0; > > -error_out: > - mutex_unlock(&sii902x->mutex); > + edid = sii902x_get_edid(sii902x, connector); > + drm_connector_update_edid_property(connector, edid); > + if (edid) { > + num = drm_add_edid_modes(connector, edid); > + kfree(edid); > + } > > - return ret; > + return num; > } > > static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, > @@ -354,12 +360,16 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > const struct drm_display_mode *adj) > { > struct sii902x *sii902x = bridge_to_sii902x(bridge); > + u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; > struct regmap *regmap = sii902x->regmap; > u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; > struct hdmi_avi_infoframe frame; > u16 pixel_clock_10kHz = adj->clock / 10; > int ret; > > + if (sii902x->sink_is_hdmi) > + output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; > + > buf[0] = pixel_clock_10kHz & 0xff; > buf[1] = pixel_clock_10kHz >> 8; > buf[2] = drm_mode_vrefresh(adj); > @@ -375,6 +385,11 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > > mutex_lock(&sii902x->mutex); > > + ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, > + SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); > + if (ret) > + goto out; > + > ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); > if (ret) > goto out; > @@ -405,13 +420,13 @@ static int sii902x_bridge_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > { > struct sii902x *sii902x = bridge_to_sii902x(bridge); > + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; > struct drm_device *drm = bridge->dev; > int ret; > > - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > - DRM_ERROR("Fix bridge driver to make connector optional!"); > - return -EINVAL; > - } > + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > + return drm_bridge_attach(bridge->encoder, sii902x->next_bridge, > + bridge, flags); > > drm_connector_helper_add(&sii902x->connector, > &sii902x_connector_helper_funcs); > @@ -433,16 +448,38 @@ static int sii902x_bridge_attach(struct drm_bridge *bridge, > else > sii902x->connector.polled = DRM_CONNECTOR_POLL_CONNECT; > > + ret = drm_display_info_set_bus_formats(&sii902x->connector.display_info, > + &bus_format, 1); > + if (ret) > + return ret; > + > drm_connector_attach_encoder(&sii902x->connector, bridge->encoder); > > return 0; > } > > +static enum drm_connector_status sii902x_bridge_detect(struct drm_bridge *bridge) > +{ > + struct sii902x *sii902x = bridge_to_sii902x(bridge); > + > + return sii902x_detect(sii902x); > +} > + > +static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge, > + struct drm_connector *connector) > +{ > + struct sii902x *sii902x = bridge_to_sii902x(bridge); > + > + return sii902x_get_edid(sii902x, connector); > +} > + > static const struct drm_bridge_funcs sii902x_bridge_funcs = { > .attach = sii902x_bridge_attach, > .mode_set = sii902x_bridge_mode_set, > .disable = sii902x_bridge_disable, > .enable = sii902x_bridge_enable, > + .detect = sii902x_bridge_detect, > + .get_edid = sii902x_bridge_get_edid, > }; > > static int sii902x_mute(struct sii902x *sii902x, bool mute) > @@ -829,8 +866,12 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) > > mutex_unlock(&sii902x->mutex); > > - if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) > + if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) { > drm_helper_hpd_irq_event(sii902x->bridge.dev); > + drm_bridge_hpd_notify(&sii902x->bridge, (status & SII902X_PLUGGED_STATUS) > + ? connector_status_connected > + : connector_status_disconnected); > + } > > return IRQ_HANDLED; > } > @@ -1001,6 +1042,11 @@ static int sii902x_init(struct sii902x *sii902x) > sii902x->bridge.funcs = &sii902x_bridge_funcs; > sii902x->bridge.of_node = dev->of_node; > sii902x->bridge.timings = &default_sii902x_timings; > + sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID; > + > + if (sii902x->i2c->irq > 0) > + sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD; > + > drm_bridge_add(&sii902x->bridge); > > sii902x_audio_codec_init(sii902x, dev); > @@ -1022,6 +1068,7 @@ static int sii902x_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > struct device *dev = &client->dev; > + struct device_node *endpoint; > struct sii902x *sii902x; > int ret; > > @@ -1049,6 +1096,28 @@ static int sii902x_probe(struct i2c_client *client, > return PTR_ERR(sii902x->reset_gpio); > } > > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); > + if (endpoint) { > + struct device_node *remote = of_graph_get_remote_port_parent(endpoint); > + > + of_node_put(endpoint); > + if (!remote) { > + dev_err(dev, "Endpoint in port@1 unconnected\n"); > + return -ENODEV; > + } > + > + if (!of_device_is_available(remote)) { > + dev_err(dev, "port@1 remote device is disabled\n"); > + of_node_put(remote); > + return -ENODEV; > + } > + > + sii902x->next_bridge = of_drm_find_bridge(remote); > + of_node_put(remote); > + if (!sii902x->next_bridge) > + return -EPROBE_DEFER; > + } > + > mutex_init(&sii902x->mutex); > > sii902x->supplies[0].supply = "iovcc"; Reviewed-by: Robert Foss <robert.foss@linaro.org> Applied to drm-misc-next.
Hi Dmitry, On 31/07/2022 22:07, Dmitry Osipenko wrote: > 13.01.2022 17:43, Neil Armstrong пишет: >> This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the >> bridge get_edid() and detect() callbacks after refactoring the connector >> get_modes() and connector_detect() callbacks. >> >> In order to keep the bridge working, extra code in get_modes() has been >> moved to more logical places. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/sii902x.c | 129 ++++++++++++++++++++++++------- >> 1 file changed, 99 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c >> index 89558e581530..65549fbfdc87 100644 >> --- a/drivers/gpu/drm/bridge/sii902x.c >> +++ b/drivers/gpu/drm/bridge/sii902x.c >> @@ -166,10 +166,12 @@ struct sii902x { >> struct i2c_client *i2c; >> struct regmap *regmap; >> struct drm_bridge bridge; >> + struct drm_bridge *next_bridge; >> struct drm_connector connector; >> struct gpio_desc *reset_gpio; >> struct i2c_mux_core *i2cmux; >> struct regulator_bulk_data supplies[2]; >> + bool sink_is_hdmi; >> /* >> * Mutex protects audio and video functions from interfering >> * each other, by keeping their i2c command sequences atomic. >> @@ -245,10 +247,8 @@ static void sii902x_reset(struct sii902x *sii902x) >> gpiod_set_value(sii902x->reset_gpio, 0); >> } >> >> -static enum drm_connector_status >> -sii902x_connector_detect(struct drm_connector *connector, bool force) >> +static enum drm_connector_status sii902x_detect(struct sii902x *sii902x) >> { >> - struct sii902x *sii902x = connector_to_sii902x(connector); >> unsigned int status; >> >> mutex_lock(&sii902x->mutex); >> @@ -261,6 +261,14 @@ sii902x_connector_detect(struct drm_connector *connector, bool force) >> connector_status_connected : connector_status_disconnected; >> } >> >> +static enum drm_connector_status >> +sii902x_connector_detect(struct drm_connector *connector, bool force) >> +{ >> + struct sii902x *sii902x = connector_to_sii902x(connector); >> + >> + return sii902x_detect(sii902x); >> +} >> + >> static const struct drm_connector_funcs sii902x_connector_funcs = { >> .detect = sii902x_connector_detect, >> .fill_modes = drm_helper_probe_single_connector_modes, >> @@ -270,42 +278,40 @@ static const struct drm_connector_funcs sii902x_connector_funcs = { >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> }; >> >> -static int sii902x_get_modes(struct drm_connector *connector) >> +static struct edid *sii902x_get_edid(struct sii902x *sii902x, >> + struct drm_connector *connector) >> { >> - struct sii902x *sii902x = connector_to_sii902x(connector); >> - u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> - u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; >> struct edid *edid; >> - int num = 0, ret; >> >> mutex_lock(&sii902x->mutex); >> >> edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); >> - drm_connector_update_edid_property(connector, edid); >> if (edid) { >> if (drm_detect_hdmi_monitor(edid)) >> - output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; >> - >> - num = drm_add_edid_modes(connector, edid); >> - kfree(edid); >> + sii902x->sink_is_hdmi = true; >> + else >> + sii902x->sink_is_hdmi = false; >> } >> >> - ret = drm_display_info_set_bus_formats(&connector->display_info, >> - &bus_format, 1); >> - if (ret) >> - goto error_out; >> + mutex_unlock(&sii902x->mutex); >> >> - ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, >> - SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); >> - if (ret) >> - goto error_out; >> + return edid; >> +} >> >> - ret = num; >> +static int sii902x_get_modes(struct drm_connector *connector) >> +{ >> + struct sii902x *sii902x = connector_to_sii902x(connector); >> + struct edid *edid; >> + int num = 0; >> >> -error_out: >> - mutex_unlock(&sii902x->mutex); >> + edid = sii902x_get_edid(sii902x, connector); >> + drm_connector_update_edid_property(connector, edid); >> + if (edid) { >> + num = drm_add_edid_modes(connector, edid); >> + kfree(edid); >> + } >> >> - return ret; >> + return num; >> } >> >> static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, >> @@ -354,12 +360,16 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, >> const struct drm_display_mode *adj) >> { >> struct sii902x *sii902x = bridge_to_sii902x(bridge); >> + u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; >> struct regmap *regmap = sii902x->regmap; >> u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; >> struct hdmi_avi_infoframe frame; >> u16 pixel_clock_10kHz = adj->clock / 10; >> int ret; >> >> + if (sii902x->sink_is_hdmi) >> + output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; >> + >> buf[0] = pixel_clock_10kHz & 0xff; >> buf[1] = pixel_clock_10kHz >> 8; >> buf[2] = drm_mode_vrefresh(adj); >> @@ -375,6 +385,11 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, >> >> mutex_lock(&sii902x->mutex); >> >> + ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, >> + SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); >> + if (ret) >> + goto out; >> + >> ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); >> if (ret) >> goto out; >> @@ -405,13 +420,13 @@ static int sii902x_bridge_attach(struct drm_bridge *bridge, >> enum drm_bridge_attach_flags flags) >> { >> struct sii902x *sii902x = bridge_to_sii902x(bridge); >> + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> struct drm_device *drm = bridge->dev; >> int ret; >> >> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { >> - DRM_ERROR("Fix bridge driver to make connector optional!"); >> - return -EINVAL; >> - } >> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >> + return drm_bridge_attach(bridge->encoder, sii902x->next_bridge, >> + bridge, flags); >> >> drm_connector_helper_add(&sii902x->connector, >> &sii902x_connector_helper_funcs); >> @@ -433,16 +448,38 @@ static int sii902x_bridge_attach(struct drm_bridge *bridge, >> else >> sii902x->connector.polled = DRM_CONNECTOR_POLL_CONNECT; >> >> + ret = drm_display_info_set_bus_formats(&sii902x->connector.display_info, >> + &bus_format, 1); >> + if (ret) >> + return ret; >> + >> drm_connector_attach_encoder(&sii902x->connector, bridge->encoder); >> >> return 0; >> } >> >> +static enum drm_connector_status sii902x_bridge_detect(struct drm_bridge *bridge) >> +{ >> + struct sii902x *sii902x = bridge_to_sii902x(bridge); >> + >> + return sii902x_detect(sii902x); >> +} >> + >> +static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge, >> + struct drm_connector *connector) >> +{ >> + struct sii902x *sii902x = bridge_to_sii902x(bridge); >> + >> + return sii902x_get_edid(sii902x, connector); >> +} >> + >> static const struct drm_bridge_funcs sii902x_bridge_funcs = { >> .attach = sii902x_bridge_attach, >> .mode_set = sii902x_bridge_mode_set, >> .disable = sii902x_bridge_disable, >> .enable = sii902x_bridge_enable, >> + .detect = sii902x_bridge_detect, >> + .get_edid = sii902x_bridge_get_edid, >> }; >> >> static int sii902x_mute(struct sii902x *sii902x, bool mute) >> @@ -829,8 +866,12 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) >> >> mutex_unlock(&sii902x->mutex); >> >> - if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) >> + if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) { >> drm_helper_hpd_irq_event(sii902x->bridge.dev); >> + drm_bridge_hpd_notify(&sii902x->bridge, (status & SII902X_PLUGGED_STATUS) >> + ? connector_status_connected >> + : connector_status_disconnected); >> + } >> >> return IRQ_HANDLED; >> } >> @@ -1001,6 +1042,11 @@ static int sii902x_init(struct sii902x *sii902x) >> sii902x->bridge.funcs = &sii902x_bridge_funcs; >> sii902x->bridge.of_node = dev->of_node; >> sii902x->bridge.timings = &default_sii902x_timings; >> + sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID; >> + >> + if (sii902x->i2c->irq > 0) >> + sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD; >> + >> drm_bridge_add(&sii902x->bridge); >> >> sii902x_audio_codec_init(sii902x, dev); >> @@ -1022,6 +1068,7 @@ static int sii902x_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> struct device *dev = &client->dev; >> + struct device_node *endpoint; >> struct sii902x *sii902x; >> int ret; >> >> @@ -1049,6 +1096,28 @@ static int sii902x_probe(struct i2c_client *client, >> return PTR_ERR(sii902x->reset_gpio); >> } >> >> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); >> + if (endpoint) { >> + struct device_node *remote = of_graph_get_remote_port_parent(endpoint); >> + >> + of_node_put(endpoint); >> + if (!remote) { >> + dev_err(dev, "Endpoint in port@1 unconnected\n"); >> + return -ENODEV; >> + } >> + >> + if (!of_device_is_available(remote)) { >> + dev_err(dev, "port@1 remote device is disabled\n"); >> + of_node_put(remote); >> + return -ENODEV; >> + } >> + >> + sii902x->next_bridge = of_drm_find_bridge(remote); >> + of_node_put(remote); >> + if (!sii902x->next_bridge) >> + return -EPROBE_DEFER; > > Hi, > > This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() > always fail with -EPROBE_DEFER. Reverting this patch returns display > back. Please fix or revert, thanks in advance. Can you share a QEMU cmdline to reproduce ? Neil
On 08/08/2022 11:15, Neil Armstrong wrote: > Hi Dmitry, > > On 31/07/2022 22:07, Dmitry Osipenko wrote: >> 13.01.2022 17:43, Neil Armstrong пишет: >>> This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the >>> bridge get_edid() and detect() callbacks after refactoring the connector >>> get_modes() and connector_detect() callbacks. >>> >>> In order to keep the bridge working, extra code in get_modes() has been >>> moved to more logical places. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/gpu/drm/bridge/sii902x.c | 129 ++++++++++++++++++++++++------- > > 1 file changed, 99 insertions(+), 30 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c >>> index 89558e581530..65549fbfdc87 100644 >>> --- a/drivers/gpu/drm/bridge/sii902x.c >>> +++ b/drivers/gpu/drm/bridge/sii902x.c [...] >>> } >>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); >>> + if (endpoint) { >>> + struct device_node *remote = of_graph_get_remote_port_parent(endpoint); >>> + >>> + of_node_put(endpoint); >>> + if (!remote) { >>> + dev_err(dev, "Endpoint in port@1 unconnected\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (!of_device_is_available(remote)) { >>> + dev_err(dev, "port@1 remote device is disabled\n"); >>> + of_node_put(remote); >>> + return -ENODEV; >>> + } >>> + >>> + sii902x->next_bridge = of_drm_find_bridge(remote); >>> + of_node_put(remote); >>> + if (!sii902x->next_bridge) >>> + return -EPROBE_DEFER; >> >> Hi, >> >> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() >> always fail with -EPROBE_DEFER. Reverting this patch returns display >> back. Please fix or revert, thanks in advance. > > Can you share a QEMU cmdline to reproduce ? Actually the vexpress DT has multiple input ports instead of a single input port at @0 and an output port at @1 like documented in the bindings: vexpress-v2m.dtsi#L303-L307: ports { #address-cells = <1>; #size-cells = <0>; /* * Both the core tile and the motherboard routes their output * pads to this transmitter. The motherboard system controller * can select one of them as input using a mux register in * "arm,vexpress-muxfpga". The Vexpress with the CA9 core tile is * the only platform with this specific set-up. */ port@0 { reg = <0>; dvi_bridge_in_ct: endpoint { remote-endpoint = <&clcd_pads_ct>; }; }; port@1 { reg = <1>; dvi_bridge_in_mb: endpoint { remote-endpoint = <&clcd_pads_mb>; }; }; }; bindings: ports: $ref: /schemas/graph.yaml#/properties/ports properties: port@0: $ref: /schemas/graph.yaml#/properties/port description: Parallel RGB input port port@1: $ref: /schemas/graph.yaml#/properties/port description: HDMI output port port@3: $ref: /schemas/graph.yaml#/properties/port description: Sound input port The patch is conform to the bindings, the DT was working but is actually not valid. Neil > > Neil
08.08.2022 12:51, Neil Armstrong пишет: > On 08/08/2022 11:15, Neil Armstrong wrote: >> Hi Dmitry, >> >> On 31/07/2022 22:07, Dmitry Osipenko wrote: >>> 13.01.2022 17:43, Neil Armstrong пишет: >>>> This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the >>>> bridge get_edid() and detect() callbacks after refactoring the >>>> connector >>>> get_modes() and connector_detect() callbacks. >>>> >>>> In order to keep the bridge working, extra code in get_modes() has been >>>> moved to more logical places. >>>> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> drivers/gpu/drm/bridge/sii902x.c | 129 >>>> ++++++++++++++++++++++++------- >> >> 1 file changed, 99 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c >>>> b/drivers/gpu/drm/bridge/sii902x.c >>>> index 89558e581530..65549fbfdc87 100644 >>>> --- a/drivers/gpu/drm/bridge/sii902x.c >>>> +++ b/drivers/gpu/drm/bridge/sii902x.c > > [...] > >>>> } >>>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); >>>> + if (endpoint) { >>>> + struct device_node *remote = >>>> of_graph_get_remote_port_parent(endpoint); >>>> + >>>> + of_node_put(endpoint); >>>> + if (!remote) { >>>> + dev_err(dev, "Endpoint in port@1 unconnected\n"); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + if (!of_device_is_available(remote)) { >>>> + dev_err(dev, "port@1 remote device is disabled\n"); >>>> + of_node_put(remote); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + sii902x->next_bridge = of_drm_find_bridge(remote); >>>> + of_node_put(remote); >>>> + if (!sii902x->next_bridge) >>>> + return -EPROBE_DEFER; >>> >>> Hi, >>> >>> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() >>> always fail with -EPROBE_DEFER. Reverting this patch returns display >>> back. Please fix or revert, thanks in advance. >> >> Can you share a QEMU cmdline to reproduce ? > > Actually the vexpress DT has multiple input ports instead of a single > input port at @0 > and an output port at @1 like documented in the bindings: > > vexpress-v2m.dtsi#L303-L307: > ports { > #address-cells = <1>; > #size-cells = <0>; > > /* > * Both the core tile and the motherboard routes their output > * pads to this transmitter. The motherboard system controller > * can select one of them as input using a mux register in > * "arm,vexpress-muxfpga". The Vexpress with the CA9 core tile is > * the only platform with this specific set-up. > */ > port@0 { > reg = <0>; > dvi_bridge_in_ct: endpoint { > remote-endpoint = <&clcd_pads_ct>; > }; > }; > port@1 { > reg = <1>; > dvi_bridge_in_mb: endpoint { > remote-endpoint = <&clcd_pads_mb>; > }; > }; > }; > > bindings: > ports: > $ref: /schemas/graph.yaml#/properties/ports > > properties: > port@0: > $ref: /schemas/graph.yaml#/properties/port > description: Parallel RGB input port > > port@1: > $ref: /schemas/graph.yaml#/properties/port > description: HDMI output port > > port@3: > $ref: /schemas/graph.yaml#/properties/port > description: Sound input port > > The patch is conform to the bindings, the DT was working but is actually > not valid. I haven't looked closely at how to fix this properly, but if we can fix it using of_machine_is_compatible("arm,vexpress") workaround in the driver, then it will be good enough at least as a temporal fix, IMO.
08.08.2022 12:15, Neil Armstrong пишет: > Hi Dmitry, > > On 31/07/2022 22:07, Dmitry Osipenko wrote: >> 13.01.2022 17:43, Neil Armstrong пишет: >>> This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the >>> bridge get_edid() and detect() callbacks after refactoring the connector >>> get_modes() and connector_detect() callbacks. >>> >>> In order to keep the bridge working, extra code in get_modes() has been >>> moved to more logical places. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/gpu/drm/bridge/sii902x.c | 129 ++++++++++++++++++++++++------- >>> > > 1 file changed, 99 insertions(+), 30 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/sii902x.c >>> b/drivers/gpu/drm/bridge/sii902x.c >>> index 89558e581530..65549fbfdc87 100644 >>> --- a/drivers/gpu/drm/bridge/sii902x.c >>> +++ b/drivers/gpu/drm/bridge/sii902x.c >>> @@ -166,10 +166,12 @@ struct sii902x { >>> struct i2c_client *i2c; >>> struct regmap *regmap; >>> struct drm_bridge bridge; >>> + struct drm_bridge *next_bridge; >>> struct drm_connector connector; >>> struct gpio_desc *reset_gpio; >>> struct i2c_mux_core *i2cmux; >>> struct regulator_bulk_data supplies[2]; >>> + bool sink_is_hdmi; >>> /* >>> * Mutex protects audio and video functions from interfering >>> * each other, by keeping their i2c command sequences atomic. >>> @@ -245,10 +247,8 @@ static void sii902x_reset(struct sii902x *sii902x) >>> gpiod_set_value(sii902x->reset_gpio, 0); >>> } >>> -static enum drm_connector_status >>> -sii902x_connector_detect(struct drm_connector *connector, bool force) >>> +static enum drm_connector_status sii902x_detect(struct sii902x >>> *sii902x) >>> { >>> - struct sii902x *sii902x = connector_to_sii902x(connector); >>> unsigned int status; >>> mutex_lock(&sii902x->mutex); >>> @@ -261,6 +261,14 @@ sii902x_connector_detect(struct drm_connector >>> *connector, bool force) >>> connector_status_connected : connector_status_disconnected; >>> } >>> +static enum drm_connector_status >>> +sii902x_connector_detect(struct drm_connector *connector, bool force) >>> +{ >>> + struct sii902x *sii902x = connector_to_sii902x(connector); >>> + >>> + return sii902x_detect(sii902x); >>> +} >>> + >>> static const struct drm_connector_funcs sii902x_connector_funcs = { >>> .detect = sii902x_connector_detect, >>> .fill_modes = drm_helper_probe_single_connector_modes, >>> @@ -270,42 +278,40 @@ static const struct drm_connector_funcs >>> sii902x_connector_funcs = { >>> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>> }; >>> -static int sii902x_get_modes(struct drm_connector *connector) >>> +static struct edid *sii902x_get_edid(struct sii902x *sii902x, >>> + struct drm_connector *connector) >>> { >>> - struct sii902x *sii902x = connector_to_sii902x(connector); >>> - u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>> - u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; >>> struct edid *edid; >>> - int num = 0, ret; >>> mutex_lock(&sii902x->mutex); >>> edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); >>> - drm_connector_update_edid_property(connector, edid); >>> if (edid) { >>> if (drm_detect_hdmi_monitor(edid)) >>> - output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; >>> - >>> - num = drm_add_edid_modes(connector, edid); >>> - kfree(edid); >>> + sii902x->sink_is_hdmi = true; >>> + else >>> + sii902x->sink_is_hdmi = false; >>> } >>> - ret = drm_display_info_set_bus_formats(&connector->display_info, >>> - &bus_format, 1); >>> - if (ret) >>> - goto error_out; >>> + mutex_unlock(&sii902x->mutex); >>> - ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, >>> - SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); >>> - if (ret) >>> - goto error_out; >>> + return edid; >>> +} >>> - ret = num; >>> +static int sii902x_get_modes(struct drm_connector *connector) >>> +{ >>> + struct sii902x *sii902x = connector_to_sii902x(connector); >>> + struct edid *edid; >>> + int num = 0; >>> -error_out: >>> - mutex_unlock(&sii902x->mutex); >>> + edid = sii902x_get_edid(sii902x, connector); >>> + drm_connector_update_edid_property(connector, edid); >>> + if (edid) { >>> + num = drm_add_edid_modes(connector, edid); >>> + kfree(edid); >>> + } >>> - return ret; >>> + return num; >>> } >>> static enum drm_mode_status sii902x_mode_valid(struct >>> drm_connector *connector, >>> @@ -354,12 +360,16 @@ static void sii902x_bridge_mode_set(struct >>> drm_bridge *bridge, >>> const struct drm_display_mode *adj) >>> { >>> struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> + u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; >>> struct regmap *regmap = sii902x->regmap; >>> u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; >>> struct hdmi_avi_infoframe frame; >>> u16 pixel_clock_10kHz = adj->clock / 10; >>> int ret; >>> + if (sii902x->sink_is_hdmi) >>> + output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; >>> + >>> buf[0] = pixel_clock_10kHz & 0xff; >>> buf[1] = pixel_clock_10kHz >> 8; >>> buf[2] = drm_mode_vrefresh(adj); >>> @@ -375,6 +385,11 @@ static void sii902x_bridge_mode_set(struct >>> drm_bridge *bridge, >>> mutex_lock(&sii902x->mutex); >>> + ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, >>> + SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); >>> + if (ret) >>> + goto out; >>> + >>> ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); >>> if (ret) >>> goto out; >>> @@ -405,13 +420,13 @@ static int sii902x_bridge_attach(struct >>> drm_bridge *bridge, >>> enum drm_bridge_attach_flags flags) >>> { >>> struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>> struct drm_device *drm = bridge->dev; >>> int ret; >>> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { >>> - DRM_ERROR("Fix bridge driver to make connector optional!"); >>> - return -EINVAL; >>> - } >>> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) >>> + return drm_bridge_attach(bridge->encoder, sii902x->next_bridge, >>> + bridge, flags); >>> drm_connector_helper_add(&sii902x->connector, >>> &sii902x_connector_helper_funcs); >>> @@ -433,16 +448,38 @@ static int sii902x_bridge_attach(struct >>> drm_bridge *bridge, >>> else >>> sii902x->connector.polled = DRM_CONNECTOR_POLL_CONNECT; >>> + ret = >>> drm_display_info_set_bus_formats(&sii902x->connector.display_info, >>> + &bus_format, 1); >>> + if (ret) >>> + return ret; >>> + >>> drm_connector_attach_encoder(&sii902x->connector, >>> bridge->encoder); >>> return 0; >>> } >>> +static enum drm_connector_status sii902x_bridge_detect(struct >>> drm_bridge *bridge) >>> +{ >>> + struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> + >>> + return sii902x_detect(sii902x); >>> +} >>> + >>> +static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge, >>> + struct drm_connector *connector) >>> +{ >>> + struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> + >>> + return sii902x_get_edid(sii902x, connector); >>> +} >>> + >>> static const struct drm_bridge_funcs sii902x_bridge_funcs = { >>> .attach = sii902x_bridge_attach, >>> .mode_set = sii902x_bridge_mode_set, >>> .disable = sii902x_bridge_disable, >>> .enable = sii902x_bridge_enable, >>> + .detect = sii902x_bridge_detect, >>> + .get_edid = sii902x_bridge_get_edid, >>> }; >>> static int sii902x_mute(struct sii902x *sii902x, bool mute) >>> @@ -829,8 +866,12 @@ static irqreturn_t sii902x_interrupt(int irq, >>> void *data) >>> mutex_unlock(&sii902x->mutex); >>> - if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) >>> + if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) { >>> drm_helper_hpd_irq_event(sii902x->bridge.dev); >>> + drm_bridge_hpd_notify(&sii902x->bridge, (status & >>> SII902X_PLUGGED_STATUS) >>> + ? connector_status_connected >>> + : connector_status_disconnected); >>> + } >>> return IRQ_HANDLED; >>> } >>> @@ -1001,6 +1042,11 @@ static int sii902x_init(struct sii902x *sii902x) >>> sii902x->bridge.funcs = &sii902x_bridge_funcs; >>> sii902x->bridge.of_node = dev->of_node; >>> sii902x->bridge.timings = &default_sii902x_timings; >>> + sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID; >>> + >>> + if (sii902x->i2c->irq > 0) >>> + sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD; >>> + >>> drm_bridge_add(&sii902x->bridge); >>> sii902x_audio_codec_init(sii902x, dev); >>> @@ -1022,6 +1068,7 @@ static int sii902x_probe(struct i2c_client >>> *client, >>> const struct i2c_device_id *id) >>> { >>> struct device *dev = &client->dev; >>> + struct device_node *endpoint; >>> struct sii902x *sii902x; >>> int ret; >>> @@ -1049,6 +1096,28 @@ static int sii902x_probe(struct i2c_client >>> *client, >>> return PTR_ERR(sii902x->reset_gpio); >>> } >>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); >>> + if (endpoint) { >>> + struct device_node *remote = >>> of_graph_get_remote_port_parent(endpoint); >>> + >>> + of_node_put(endpoint); >>> + if (!remote) { >>> + dev_err(dev, "Endpoint in port@1 unconnected\n"); >>> + return -ENODEV; >>> + } >>> + >>> + if (!of_device_is_available(remote)) { >>> + dev_err(dev, "port@1 remote device is disabled\n"); >>> + of_node_put(remote); >>> + return -ENODEV; >>> + } >>> + >>> + sii902x->next_bridge = of_drm_find_bridge(remote); >>> + of_node_put(remote); >>> + if (!sii902x->next_bridge) >>> + return -EPROBE_DEFER; >> >> Hi, >> >> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() >> always fail with -EPROBE_DEFER. Reverting this patch returns display >> back. Please fix or revert, thanks in advance. > > Can you share a QEMU cmdline to reproduce ? qemu-system-arm -cpu cortex-a9 -M vexpress-a9 -smp 2 -m 1024 -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/vexpress-v2p-ca9.dtb -serial stdio -net nic,model=lan9118 -net user
On 15/08/2022 02:15, Dmitry Osipenko wrote: > 08.08.2022 12:51, Neil Armstrong пишет: >> On 08/08/2022 11:15, Neil Armstrong wrote: >>> Hi Dmitry, >>> >>> On 31/07/2022 22:07, Dmitry Osipenko wrote: >>>> 13.01.2022 17:43, Neil Armstrong пишет: >>>>> This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the >>>>> bridge get_edid() and detect() callbacks after refactoring the >>>>> connector >>>>> get_modes() and connector_detect() callbacks. >>>>> >>>>> In order to keep the bridge working, extra code in get_modes() has been >>>>> moved to more logical places. >>>>> >>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>>> --- >>>>> drivers/gpu/drm/bridge/sii902x.c | 129 >>>>> ++++++++++++++++++++++++------- >>> >>> 1 file changed, 99 insertions(+), 30 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/sii902x.c >>>>> b/drivers/gpu/drm/bridge/sii902x.c >>>>> index 89558e581530..65549fbfdc87 100644 >>>>> --- a/drivers/gpu/drm/bridge/sii902x.c >>>>> +++ b/drivers/gpu/drm/bridge/sii902x.c >> >> [...] >> >>>>> } >>>>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); >>>>> + if (endpoint) { >>>>> + struct device_node *remote = >>>>> of_graph_get_remote_port_parent(endpoint); >>>>> + >>>>> + of_node_put(endpoint); >>>>> + if (!remote) { >>>>> + dev_err(dev, "Endpoint in port@1 unconnected\n"); >>>>> + return -ENODEV; >>>>> + } >>>>> + >>>>> + if (!of_device_is_available(remote)) { >>>>> + dev_err(dev, "port@1 remote device is disabled\n"); >>>>> + of_node_put(remote); >>>>> + return -ENODEV; >>>>> + } >>>>> + >>>>> + sii902x->next_bridge = of_drm_find_bridge(remote); >>>>> + of_node_put(remote); >>>>> + if (!sii902x->next_bridge) >>>>> + return -EPROBE_DEFER; >>>> >>>> Hi, >>>> >>>> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() >>>> always fail with -EPROBE_DEFER. Reverting this patch returns display >>>> back. Please fix or revert, thanks in advance. >>> >>> Can you share a QEMU cmdline to reproduce ? >> >> Actually the vexpress DT has multiple input ports instead of a single >> input port at @0 >> and an output port at @1 like documented in the bindings: >> >> vexpress-v2m.dtsi#L303-L307: >> ports { >> #address-cells = <1>; >> #size-cells = <0>; >> >> /* >> * Both the core tile and the motherboard routes their output >> * pads to this transmitter. The motherboard system controller >> * can select one of them as input using a mux register in >> * "arm,vexpress-muxfpga". The Vexpress with the CA9 core tile is >> * the only platform with this specific set-up. >> */ >> port@0 { >> reg = <0>; >> dvi_bridge_in_ct: endpoint { >> remote-endpoint = <&clcd_pads_ct>; >> }; >> }; >> port@1 { >> reg = <1>; >> dvi_bridge_in_mb: endpoint { >> remote-endpoint = <&clcd_pads_mb>; >> }; >> }; >> }; >> >> bindings: >> ports: >> $ref: /schemas/graph.yaml#/properties/ports >> >> properties: >> port@0: >> $ref: /schemas/graph.yaml#/properties/port >> description: Parallel RGB input port >> >> port@1: >> $ref: /schemas/graph.yaml#/properties/port >> description: HDMI output port >> >> port@3: >> $ref: /schemas/graph.yaml#/properties/port >> description: Sound input port >> >> The patch is conform to the bindings, the DT was working but is actually >> not valid. > > I haven't looked closely at how to fix this properly, but if we can fix > it using of_machine_is_compatible("arm,vexpress") workaround in the > driver, then it will be good enough at least as a temporal fix, IMO. If other maintainers are ok with that, it can be temporary fix until the DT gets fixed. Neil
On Wed, Aug 17, 2022 at 3:31 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > On 15/08/2022 02:15, Dmitry Osipenko wrote: > > 08.08.2022 12:51, Neil Armstrong пишет: > >> On 08/08/2022 11:15, Neil Armstrong wrote: > >>>> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() > >>>> always fail with -EPROBE_DEFER. Reverting this patch returns display > >>>> back. Please fix or revert, thanks in advance. > >>> > >>> Can you share a QEMU cmdline to reproduce ? > >> > >> Actually the vexpress DT has multiple input ports instead of a single > >> input port at @0 > >> and an output port at @1 like documented in the bindings: > >> > >> vexpress-v2m.dtsi#L303-L307: > >> ports { > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> /* > >> * Both the core tile and the motherboard routes their output > >> * pads to this transmitter. The motherboard system controller > >> * can select one of them as input using a mux register in > >> * "arm,vexpress-muxfpga". The Vexpress with the CA9 core tile is > >> * the only platform with this specific set-up. > >> */ > >> port@0 { > >> reg = <0>; > >> dvi_bridge_in_ct: endpoint { > >> remote-endpoint = <&clcd_pads_ct>; > >> }; > >> }; > >> port@1 { > >> reg = <1>; > >> dvi_bridge_in_mb: endpoint { > >> remote-endpoint = <&clcd_pads_mb>; > >> }; > >> }; > >> }; > >> > >> bindings: > >> ports: > >> $ref: /schemas/graph.yaml#/properties/ports > >> > >> properties: > >> port@0: > >> $ref: /schemas/graph.yaml#/properties/port > >> description: Parallel RGB input port > >> > >> port@1: > >> $ref: /schemas/graph.yaml#/properties/port > >> description: HDMI output port > >> > >> port@3: > >> $ref: /schemas/graph.yaml#/properties/port > >> description: Sound input port > >> > >> The patch is conform to the bindings, the DT was working but is actually > >> not valid. > > > > I haven't looked closely at how to fix this properly, but if we can fix > > it using of_machine_is_compatible("arm,vexpress") workaround in the > > driver, then it will be good enough at least as a temporal fix, IMO. > > If other maintainers are ok with that, it can be temporary fix until the DT gets fixed. That's fine with me, will you send a patch? I don't know how you expect the DT to get "fixed" though. The hardware looks like this, it's maybe not the most elegant electronics design but it exists, so... I wrote this DT with two inputs, see commit f1fe12c8bf332, the code handling this awkward mux is part of the DRM driver, see drivers/gpu/drm/pl111/pl111_versatile.c function pl111_vexpress_clcd_init() for an idea of how it works. Yours, Linus Walleij
On 25/08/2022 14:48, Linus Walleij wrote: > On Wed, Aug 17, 2022 at 3:31 PM Neil Armstrong <narmstrong@baylibre.com> wrote: >> On 15/08/2022 02:15, Dmitry Osipenko wrote: >>> 08.08.2022 12:51, Neil Armstrong пишет: >>>> On 08/08/2022 11:15, Neil Armstrong wrote: > >>>>>> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() >>>>>> always fail with -EPROBE_DEFER. Reverting this patch returns display >>>>>> back. Please fix or revert, thanks in advance. >>>>> >>>>> Can you share a QEMU cmdline to reproduce ? >>>> >>>> Actually the vexpress DT has multiple input ports instead of a single >>>> input port at @0 >>>> and an output port at @1 like documented in the bindings: >>>> >>>> vexpress-v2m.dtsi#L303-L307: >>>> ports { >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> >>>> /* >>>> * Both the core tile and the motherboard routes their output >>>> * pads to this transmitter. The motherboard system controller >>>> * can select one of them as input using a mux register in >>>> * "arm,vexpress-muxfpga". The Vexpress with the CA9 core tile is >>>> * the only platform with this specific set-up. >>>> */ >>>> port@0 { >>>> reg = <0>; >>>> dvi_bridge_in_ct: endpoint { >>>> remote-endpoint = <&clcd_pads_ct>; >>>> }; >>>> }; >>>> port@1 { >>>> reg = <1>; >>>> dvi_bridge_in_mb: endpoint { >>>> remote-endpoint = <&clcd_pads_mb>; >>>> }; >>>> }; >>>> }; >>>> >>>> bindings: >>>> ports: >>>> $ref: /schemas/graph.yaml#/properties/ports >>>> >>>> properties: >>>> port@0: >>>> $ref: /schemas/graph.yaml#/properties/port >>>> description: Parallel RGB input port >>>> >>>> port@1: >>>> $ref: /schemas/graph.yaml#/properties/port >>>> description: HDMI output port >>>> >>>> port@3: >>>> $ref: /schemas/graph.yaml#/properties/port >>>> description: Sound input port >>>> >>>> The patch is conform to the bindings, the DT was working but is actually >>>> not valid. >>> >>> I haven't looked closely at how to fix this properly, but if we can fix >>> it using of_machine_is_compatible("arm,vexpress") workaround in the >>> driver, then it will be good enough at least as a temporal fix, IMO. >> >> If other maintainers are ok with that, it can be temporary fix until the DT gets fixed. > > That's fine with me, will you send a patch? Who, me ? > > I don't know how you expect the DT to get "fixed" though. > > The hardware looks like this, it's maybe not the most elegant > electronics design but it exists, so... I wrote this DT with two > inputs, see commit f1fe12c8bf332, the code handling this > awkward mux is part of the DRM driver, see > drivers/gpu/drm/pl111/pl111_versatile.c function > pl111_vexpress_clcd_init() for an idea of how it works. The proper fix would be the other way around, adding a mux bridge before the sii902x returning the next bridge or nothing to the right controller. > > Yours, > Linus Walleij Neil
On Mon, Aug 29, 2022 at 3:36 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > On 25/08/2022 14:48, Linus Walleij wrote: > > On Wed, Aug 17, 2022 at 3:31 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > >> On 15/08/2022 02:15, Dmitry Osipenko wrote: > >>> 08.08.2022 12:51, Neil Armstrong пишет: > >>>> On 08/08/2022 11:15, Neil Armstrong wrote: > > > >>>>>> This patch broke ARM/QEMU vexpress display because of_drm_find_bridge() > >>>>>> always fail with -EPROBE_DEFER. Reverting this patch returns display > >>>>>> back. Please fix or revert, thanks in advance. > >>>>> > >>>>> Can you share a QEMU cmdline to reproduce ? > >>>> > >>>> Actually the vexpress DT has multiple input ports instead of a single > >>>> input port at @0 > >>>> and an output port at @1 like documented in the bindings: > >>>> > >>>> vexpress-v2m.dtsi#L303-L307: > >>>> ports { > >>>> #address-cells = <1>; > >>>> #size-cells = <0>; > >>>> > >>>> /* > >>>> * Both the core tile and the motherboard routes their output > >>>> * pads to this transmitter. The motherboard system controller > >>>> * can select one of them as input using a mux register in > >>>> * "arm,vexpress-muxfpga". The Vexpress with the CA9 core tile is > >>>> * the only platform with this specific set-up. > >>>> */ > >>>> port@0 { > >>>> reg = <0>; > >>>> dvi_bridge_in_ct: endpoint { > >>>> remote-endpoint = <&clcd_pads_ct>; > >>>> }; > >>>> }; > >>>> port@1 { > >>>> reg = <1>; > >>>> dvi_bridge_in_mb: endpoint { > >>>> remote-endpoint = <&clcd_pads_mb>; > >>>> }; > >>>> }; > >>>> }; > >>>> > >>>> bindings: > >>>> ports: > >>>> $ref: /schemas/graph.yaml#/properties/ports > >>>> > >>>> properties: > >>>> port@0: > >>>> $ref: /schemas/graph.yaml#/properties/port > >>>> description: Parallel RGB input port > >>>> > >>>> port@1: > >>>> $ref: /schemas/graph.yaml#/properties/port > >>>> description: HDMI output port > >>>> > >>>> port@3: > >>>> $ref: /schemas/graph.yaml#/properties/port > >>>> description: Sound input port > >>>> > >>>> The patch is conform to the bindings, the DT was working but is actually > >>>> not valid. > >>> > >>> I haven't looked closely at how to fix this properly, but if we can fix > >>> it using of_machine_is_compatible("arm,vexpress") workaround in the > >>> driver, then it will be good enough at least as a temporal fix, IMO. > >> > >> If other maintainers are ok with that, it can be temporary fix until the DT gets fixed. > > > > That's fine with me, will you send a patch? > > Who, me ? Whoever you were referring to with the "temporary fix" :D > > I don't know how you expect the DT to get "fixed" though. > > > > The hardware looks like this, it's maybe not the most elegant > > electronics design but it exists, so... I wrote this DT with two > > inputs, see commit f1fe12c8bf332, the code handling this > > awkward mux is part of the DRM driver, see > > drivers/gpu/drm/pl111/pl111_versatile.c function > > pl111_vexpress_clcd_init() for an idea of how it works. > > The proper fix would be the other way around, adding a mux bridge before the sii902x > returning the next bridge or nothing to the right controller. Hm you mean the vexpress PLD should be a bridge and not just some magic registers poked by the DRM driver? OK fair point. But it will need proper representing in the DT then, I guess that is what you mean by "DT gets fixed". It's not just the DT that needs fixing then but also the driver(s). Yours, Linus Walleij
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index 89558e581530..65549fbfdc87 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -166,10 +166,12 @@ struct sii902x { struct i2c_client *i2c; struct regmap *regmap; struct drm_bridge bridge; + struct drm_bridge *next_bridge; struct drm_connector connector; struct gpio_desc *reset_gpio; struct i2c_mux_core *i2cmux; struct regulator_bulk_data supplies[2]; + bool sink_is_hdmi; /* * Mutex protects audio and video functions from interfering * each other, by keeping their i2c command sequences atomic. @@ -245,10 +247,8 @@ static void sii902x_reset(struct sii902x *sii902x) gpiod_set_value(sii902x->reset_gpio, 0); } -static enum drm_connector_status -sii902x_connector_detect(struct drm_connector *connector, bool force) +static enum drm_connector_status sii902x_detect(struct sii902x *sii902x) { - struct sii902x *sii902x = connector_to_sii902x(connector); unsigned int status; mutex_lock(&sii902x->mutex); @@ -261,6 +261,14 @@ sii902x_connector_detect(struct drm_connector *connector, bool force) connector_status_connected : connector_status_disconnected; } +static enum drm_connector_status +sii902x_connector_detect(struct drm_connector *connector, bool force) +{ + struct sii902x *sii902x = connector_to_sii902x(connector); + + return sii902x_detect(sii902x); +} + static const struct drm_connector_funcs sii902x_connector_funcs = { .detect = sii902x_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, @@ -270,42 +278,40 @@ static const struct drm_connector_funcs sii902x_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; -static int sii902x_get_modes(struct drm_connector *connector) +static struct edid *sii902x_get_edid(struct sii902x *sii902x, + struct drm_connector *connector) { - struct sii902x *sii902x = connector_to_sii902x(connector); - u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; - u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; struct edid *edid; - int num = 0, ret; mutex_lock(&sii902x->mutex); edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); - drm_connector_update_edid_property(connector, edid); if (edid) { if (drm_detect_hdmi_monitor(edid)) - output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; - - num = drm_add_edid_modes(connector, edid); - kfree(edid); + sii902x->sink_is_hdmi = true; + else + sii902x->sink_is_hdmi = false; } - ret = drm_display_info_set_bus_formats(&connector->display_info, - &bus_format, 1); - if (ret) - goto error_out; + mutex_unlock(&sii902x->mutex); - ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, - SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); - if (ret) - goto error_out; + return edid; +} - ret = num; +static int sii902x_get_modes(struct drm_connector *connector) +{ + struct sii902x *sii902x = connector_to_sii902x(connector); + struct edid *edid; + int num = 0; -error_out: - mutex_unlock(&sii902x->mutex); + edid = sii902x_get_edid(sii902x, connector); + drm_connector_update_edid_property(connector, edid); + if (edid) { + num = drm_add_edid_modes(connector, edid); + kfree(edid); + } - return ret; + return num; } static enum drm_mode_status sii902x_mode_valid(struct drm_connector *connector, @@ -354,12 +360,16 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, const struct drm_display_mode *adj) { struct sii902x *sii902x = bridge_to_sii902x(bridge); + u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; struct regmap *regmap = sii902x->regmap; u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; struct hdmi_avi_infoframe frame; u16 pixel_clock_10kHz = adj->clock / 10; int ret; + if (sii902x->sink_is_hdmi) + output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; + buf[0] = pixel_clock_10kHz & 0xff; buf[1] = pixel_clock_10kHz >> 8; buf[2] = drm_mode_vrefresh(adj); @@ -375,6 +385,11 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, mutex_lock(&sii902x->mutex); + ret = regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, + SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); + if (ret) + goto out; + ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); if (ret) goto out; @@ -405,13 +420,13 @@ static int sii902x_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct sii902x *sii902x = bridge_to_sii902x(bridge); + u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; struct drm_device *drm = bridge->dev; int ret; - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return drm_bridge_attach(bridge->encoder, sii902x->next_bridge, + bridge, flags); drm_connector_helper_add(&sii902x->connector, &sii902x_connector_helper_funcs); @@ -433,16 +448,38 @@ static int sii902x_bridge_attach(struct drm_bridge *bridge, else sii902x->connector.polled = DRM_CONNECTOR_POLL_CONNECT; + ret = drm_display_info_set_bus_formats(&sii902x->connector.display_info, + &bus_format, 1); + if (ret) + return ret; + drm_connector_attach_encoder(&sii902x->connector, bridge->encoder); return 0; } +static enum drm_connector_status sii902x_bridge_detect(struct drm_bridge *bridge) +{ + struct sii902x *sii902x = bridge_to_sii902x(bridge); + + return sii902x_detect(sii902x); +} + +static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge, + struct drm_connector *connector) +{ + struct sii902x *sii902x = bridge_to_sii902x(bridge); + + return sii902x_get_edid(sii902x, connector); +} + static const struct drm_bridge_funcs sii902x_bridge_funcs = { .attach = sii902x_bridge_attach, .mode_set = sii902x_bridge_mode_set, .disable = sii902x_bridge_disable, .enable = sii902x_bridge_enable, + .detect = sii902x_bridge_detect, + .get_edid = sii902x_bridge_get_edid, }; static int sii902x_mute(struct sii902x *sii902x, bool mute) @@ -829,8 +866,12 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) mutex_unlock(&sii902x->mutex); - if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) + if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) { drm_helper_hpd_irq_event(sii902x->bridge.dev); + drm_bridge_hpd_notify(&sii902x->bridge, (status & SII902X_PLUGGED_STATUS) + ? connector_status_connected + : connector_status_disconnected); + } return IRQ_HANDLED; } @@ -1001,6 +1042,11 @@ static int sii902x_init(struct sii902x *sii902x) sii902x->bridge.funcs = &sii902x_bridge_funcs; sii902x->bridge.of_node = dev->of_node; sii902x->bridge.timings = &default_sii902x_timings; + sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID; + + if (sii902x->i2c->irq > 0) + sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD; + drm_bridge_add(&sii902x->bridge); sii902x_audio_codec_init(sii902x, dev); @@ -1022,6 +1068,7 @@ static int sii902x_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct device *dev = &client->dev; + struct device_node *endpoint; struct sii902x *sii902x; int ret; @@ -1049,6 +1096,28 @@ static int sii902x_probe(struct i2c_client *client, return PTR_ERR(sii902x->reset_gpio); } + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1); + if (endpoint) { + struct device_node *remote = of_graph_get_remote_port_parent(endpoint); + + of_node_put(endpoint); + if (!remote) { + dev_err(dev, "Endpoint in port@1 unconnected\n"); + return -ENODEV; + } + + if (!of_device_is_available(remote)) { + dev_err(dev, "port@1 remote device is disabled\n"); + of_node_put(remote); + return -ENODEV; + } + + sii902x->next_bridge = of_drm_find_bridge(remote); + of_node_put(remote); + if (!sii902x->next_bridge) + return -EPROBE_DEFER; + } + mutex_init(&sii902x->mutex); sii902x->supplies[0].supply = "iovcc";
This adds support for DRM_BRIDGE_ATTACH_NO_CONNECTOR by adding the bridge get_edid() and detect() callbacks after refactoring the connector get_modes() and connector_detect() callbacks. In order to keep the bridge working, extra code in get_modes() has been moved to more logical places. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/sii902x.c | 129 ++++++++++++++++++++++++------- 1 file changed, 99 insertions(+), 30 deletions(-)