| Message ID | 20210322030128.2283-12-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand |
Quoting Laurent Pinchart (2021-03-21 20:01:28) > When the SN65DSI86 is used in DisplayPort mode, its output is likely > routed to a DisplayPort connector, which can benefit from hotplug > detection. Support it in such cases, with polling mode only for now. > > The implementation is limited to the bridge operations, as the connector > operations are legacy and new users should use > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++-------- > 1 file changed, 33 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index f792227142a7..72f6362adf44 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > pm_runtime_put_sync(pdata->dev); > } > > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) > +{ > + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); > + int val; > + > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val); > + return val ? connector_status_connected : connector_status_disconnected; > +} > + > static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, > struct drm_connector *connector) > { > @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > .enable = ti_sn_bridge_enable, > .disable = ti_sn_bridge_disable, > .post_disable = ti_sn_bridge_post_disable, > + .detect = ti_sn_bridge_detect, > .get_edid = ti_sn_bridge_get_edid, > }; > > @@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > return ret; > } > > + pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd"); I see that we missed adding this property to the DTS file but skated by because it was the default in the driver. I don't think it's a big deal just something we should fix in sc7180-trogdor.dtsi before this patch is merged. > + > ti_sn_bridge_parse_lanes(pdata, client->dev.of_node); > > ret = ti_sn_bridge_parse_regulators(pdata);
Hi, On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > When the SN65DSI86 is used in DisplayPort mode, its output is likely > routed to a DisplayPort connector, which can benefit from hotplug > detection. Support it in such cases, with polling mode only for now. > > The implementation is limited to the bridge operations, as the connector > operations are legacy and new users should use > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++-------- > 1 file changed, 33 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index f792227142a7..72f6362adf44 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -167,6 +167,8 @@ struct ti_sn_bridge { > struct gpio_chip gchip; > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); > #endif > + > + bool no_hpd; This structure is documented by kernel-doc, but you didn't add your new member. > }; > > static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { > @@ -862,23 +864,28 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) > ti_sn_bridge_set_refclk_freq(pdata); > > /* > - * HPD on this bridge chip is a bit useless. This is an eDP bridge > - * so the HPD is an internal signal that's only there to signal that > - * the panel is done powering up. ...but the bridge chip debounces > - * this signal by between 100 ms and 400 ms (depending on process, > - * voltage, and temperate--I measured it at about 200 ms). One > + * As this is an eDP bridge, the output will be connected to a fixed > + * panel in most systems. HPD is in that case only an internal signal > + * to signal that the panel is done powering up. The bridge chip > + * debounces this signal by between 100 ms and 400 ms (depending on > + * process, voltage, and temperate--I measured it at about 200 ms). One > * particular panel asserted HPD 84 ms after it was powered on meaning > * that we saw HPD 284 ms after power on. ...but the same panel said > * that instead of looking at HPD you could just hardcode a delay of > - * 200 ms. We'll assume that the panel driver will have the hardcoded > - * delay in its prepare and always disable HPD. > + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll > + * assume that the panel driver will have the hardcoded delay in its > + * prepare and always disable HPD. > * > - * If HPD somehow makes sense on some future panel we'll have to > - * change this to be conditional on someone specifying that HPD should > - * be used. > + * However, on some systems, the output is connected to a DisplayPort > + * connector. HPD is needed in such cases. To accommodate both use > + * cases, enable HPD only when requested. > */ > - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, > - HPD_DISABLE); > + if (pdata->no_hpd) > + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, > + HPD_DISABLE, HPD_DISABLE); > + else > + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, > + HPD_DISABLE, 0); Optionally you could skip the "else". HPD enabled is the default state and, in general, we don't exhaustively init all registers and rely on the power-on defaults for ones we don't explicitly control. > } > > static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > pm_runtime_put_sync(pdata->dev); > } > > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) > +{ > + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); > + int val; > + > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val); > + return val ? connector_status_connected : connector_status_disconnected; I would have expected that you would have used the interrupt signal, but I guess it just polls in this case. I suppose polling has the advantage that it's simpler... Maybe throw in a comment about why IRQ isn't being used? > +} > + > static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, > struct drm_connector *connector) > { > @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > .enable = ti_sn_bridge_enable, > .disable = ti_sn_bridge_disable, > .post_disable = ti_sn_bridge_post_disable, > + .detect = ti_sn_bridge_detect, > .get_edid = ti_sn_bridge_get_edid, > }; > > @@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > return ret; > } > > + pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd"); > + > ti_sn_bridge_parse_lanes(pdata, client->dev.of_node); > > ret = ti_sn_bridge_parse_regulators(pdata); > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > pdata->bridge.of_node = client->dev.of_node; > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) Checking for "no_hpd" here is not the right test IIUC. You want to check for eDP vs. DP (AKA whether a panel is downstream of you or a connector). Specifically if downstream of you is a panel then (I believe) HPD won't assert until you turn on the panel and you won't turn on the panel (which happens in pre_enable, right?) until HPD fires, so you've got a chicken-and-egg problem. If downstream of you is a connector, though, then by definition HPD has to just work without pre_enable running so then you're OK. I guess then you'd need to figure out what to do if someone wants to use "HPD" on eDP. Do you need to put a polling loop in pre_enable then? Or you could just punt not support this case until someone needs it. > + | DRM_BRIDGE_OP_EDID; IMO somewhere in here if HPD is being used like this you should throw in a call to pm_runtime_get_sync(). I guess in your solution the regulators (for the bridge, not the panel) and enable pin are just left on all the time, but plausibly someone might want to build a system to use HPD and also have the enable pin and/or regulators controlled by this driver, right? -Doug
Hi Doug, On Wed, Mar 24, 2021 at 03:47:38PM -0700, Doug Anderson wrote: > On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > > > When the SN65DSI86 is used in DisplayPort mode, its output is likely > > routed to a DisplayPort connector, which can benefit from hotplug > > detection. Support it in such cases, with polling mode only for now. > > > > The implementation is limited to the bridge operations, as the connector > > operations are legacy and new users should use > > DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++-------- > > 1 file changed, 33 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index f792227142a7..72f6362adf44 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -167,6 +167,8 @@ struct ti_sn_bridge { > > struct gpio_chip gchip; > > DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); > > #endif > > + > > + bool no_hpd; > > This structure is documented by kernel-doc, but you didn't add your new member. Oops, sorry. > > }; > > > > static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { > > @@ -862,23 +864,28 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) > > ti_sn_bridge_set_refclk_freq(pdata); > > > > /* > > - * HPD on this bridge chip is a bit useless. This is an eDP bridge > > - * so the HPD is an internal signal that's only there to signal that > > - * the panel is done powering up. ...but the bridge chip debounces > > - * this signal by between 100 ms and 400 ms (depending on process, > > - * voltage, and temperate--I measured it at about 200 ms). One > > + * As this is an eDP bridge, the output will be connected to a fixed > > + * panel in most systems. HPD is in that case only an internal signal > > + * to signal that the panel is done powering up. The bridge chip > > + * debounces this signal by between 100 ms and 400 ms (depending on > > + * process, voltage, and temperate--I measured it at about 200 ms). One > > * particular panel asserted HPD 84 ms after it was powered on meaning > > * that we saw HPD 284 ms after power on. ...but the same panel said > > * that instead of looking at HPD you could just hardcode a delay of > > - * 200 ms. We'll assume that the panel driver will have the hardcoded > > - * delay in its prepare and always disable HPD. > > + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll > > + * assume that the panel driver will have the hardcoded delay in its > > + * prepare and always disable HPD. > > * > > - * If HPD somehow makes sense on some future panel we'll have to > > - * change this to be conditional on someone specifying that HPD should > > - * be used. > > + * However, on some systems, the output is connected to a DisplayPort > > + * connector. HPD is needed in such cases. To accommodate both use > > + * cases, enable HPD only when requested. > > */ > > - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, > > - HPD_DISABLE); > > + if (pdata->no_hpd) > > + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, > > + HPD_DISABLE, HPD_DISABLE); > > + else > > + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, > > + HPD_DISABLE, 0); > > Optionally you could skip the "else". HPD enabled is the default state > and, in general, we don't exhaustively init all registers and rely on > the power-on defaults for ones we don't explicitly control. OK. > > } > > > > static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > > @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > > pm_runtime_put_sync(pdata->dev); > > } > > > > +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) > > +{ > > + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); > > + int val; > > + > > + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val); > > + return val ? connector_status_connected : connector_status_disconnected; > > I would have expected that you would have used the interrupt signal, > but I guess it just polls in this case. I suppose polling has the > advantage that it's simpler... Maybe throw in a comment about why IRQ > isn't being used? Correct, I didn't want to include IRQ support yet. I'll add a TODO comment. > > +} > > + > > static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, > > struct drm_connector *connector) > > { > > @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { > > .enable = ti_sn_bridge_enable, > > .disable = ti_sn_bridge_disable, > > .post_disable = ti_sn_bridge_post_disable, > > + .detect = ti_sn_bridge_detect, > > .get_edid = ti_sn_bridge_get_edid, > > }; u> > > > @@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > return ret; > > } > > > > + pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd"); > > + > > ti_sn_bridge_parse_lanes(pdata, client->dev.of_node); > > > > ret = ti_sn_bridge_parse_regulators(pdata); > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > pdata->bridge.of_node = client->dev.of_node; > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > Checking for "no_hpd" here is not the right test IIUC. You want to > check for eDP vs. DP (AKA whether a panel is downstream of you or a > connector). Specifically if downstream of you is a panel then (I > believe) HPD won't assert until you turn on the panel and you won't > turn on the panel (which happens in pre_enable, right?) until HPD > fires, so you've got a chicken-and-egg problem. If downstream of you > is a connector, though, then by definition HPD has to just work > without pre_enable running so then you're OK. Agreed. It's even more true now that your rework has landed, as in the eDP case EDID is handled by the panel driver. I'll rework this. Should I also condition setting HPD_DISABLE to the presence of a panel then ? I could drop of_property_read_bool() and set pdata->no_hpd = !!panel; > I guess then you'd need to figure out what to do if someone wants to > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > then? Or you could just punt not support this case until someone needs > it. I think I'll stop short of saving the world this time, yes :-) We'll see what to do when this case arises. > > + | DRM_BRIDGE_OP_EDID; > > IMO somewhere in here if HPD is being used like this you should throw > in a call to pm_runtime_get_sync(). I guess in your solution the > regulators (for the bridge, not the panel) and enable pin are just > left on all the time, Correct, on my development board the SN65DSI86 is on all the time, I can't control that. > but plausibly someone might want to build a > system to use HPD and also have the enable pin and/or regulators > controlled by this driver, right? True. DRM doesn't make this very easy, as, as far as I can tell, there's no standard infrastructure for userspace to register an interest in HPD that could be notified to bridges. I think it should be fixable, but it's out of scope for this series :-) Should I still add a pm_runtime_get_sync() at probe time, or leave this to be addressed by someone who will need to implement power control ?
Hi, On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > pdata->bridge.of_node = client->dev.of_node; > > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > > > Checking for "no_hpd" here is not the right test IIUC. You want to > > check for eDP vs. DP (AKA whether a panel is downstream of you or a > > connector). Specifically if downstream of you is a panel then (I > > believe) HPD won't assert until you turn on the panel and you won't > > turn on the panel (which happens in pre_enable, right?) until HPD > > fires, so you've got a chicken-and-egg problem. If downstream of you > > is a connector, though, then by definition HPD has to just work > > without pre_enable running so then you're OK. > > Agreed. It's even more true now that your rework has landed, as in the > eDP case EDID is handled by the panel driver. I'll rework this. > > Should I also condition setting HPD_DISABLE to the presence of a panel > then ? I could drop of_property_read_bool() and set > > pdata->no_hpd = !!panel; > > > I guess then you'd need to figure out what to do if someone wants to > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > > then? Or you could just punt not support this case until someone needs > > it. > > I think I'll stop short of saving the world this time, yes :-) We'll see > what to do when this case arises. How about as a compromise you still call of_property_read_bool() but add some type of warning in the logs if someone didn't set "no-hpd" but they have a panel? > > > + | DRM_BRIDGE_OP_EDID; > > > > IMO somewhere in here if HPD is being used like this you should throw > > in a call to pm_runtime_get_sync(). I guess in your solution the > > regulators (for the bridge, not the panel) and enable pin are just > > left on all the time, > > Correct, on my development board the SN65DSI86 is on all the time, I > can't control that. > > > but plausibly someone might want to build a > > system to use HPD and also have the enable pin and/or regulators > > controlled by this driver, right? > > True. DRM doesn't make this very easy, as, as far as I can tell, there's > no standard infrastructure for userspace to register an interest in HPD > that could be notified to bridges. I think it should be fixable, but > it's out of scope for this series :-) Should I still add a > pm_runtime_get_sync() at probe time, or leave this to be addressed by > someone who will need to implement power control ? IMO if you've detected you're running in DP mode you should add the pm_runtime_get_sync() in probe to keep it powered all the time and that seems the simplest. Technically I guess that's not really required since you're polling and you could power off between polls, but then you'd have to re-init a bunch of your state each time you polled too. If you ever transitioned to using an IRQ for HPD then you'd have to keep it always powered anyway. -Doug
Hi All, I'm working to respin the remainder of these patches, now that I have IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used for Renesas R-Car boards. Quoting Doug Anderson (2021-06-24 00:51:12) > Hi, > > On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > > pdata->bridge.of_node = client->dev.of_node; > > > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > > > > > Checking for "no_hpd" here is not the right test IIUC. You want to > > > check for eDP vs. DP (AKA whether a panel is downstream of you or a > > > connector). Specifically if downstream of you is a panel then (I > > > believe) HPD won't assert until you turn on the panel and you won't > > > turn on the panel (which happens in pre_enable, right?) until HPD > > > fires, so you've got a chicken-and-egg problem. If downstream of you > > > is a connector, though, then by definition HPD has to just work > > > without pre_enable running so then you're OK. > > > > Agreed. It's even more true now that your rework has landed, as in the > > eDP case EDID is handled by the panel driver. I'll rework this. > > > > Should I also condition setting HPD_DISABLE to the presence of a panel > > then ? I could drop of_property_read_bool() and set > > > > pdata->no_hpd = !!panel; > > > > > I guess then you'd need to figure out what to do if someone wants to > > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > > > then? Or you could just punt not support this case until someone needs > > > it. > > > > I think I'll stop short of saving the world this time, yes :-) We'll see > > what to do when this case arises. > > How about as a compromise you still call of_property_read_bool() but > add some type of warning in the logs if someone didn't set "no-hpd" > but they have a panel? Would a mix of the two work well? What about: pdata->no_hpd = of_property_read_bool(np, "no-hpd"); if (panel && !pdata->no_hpd) { DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n"); pdata->no_hpd = true; } That leaves it still optional to DP connectors, but enforced on eDP? > > > > + | DRM_BRIDGE_OP_EDID; > > > > > > IMO somewhere in here if HPD is being used like this you should throw > > > in a call to pm_runtime_get_sync(). I guess in your solution the > > > regulators (for the bridge, not the panel) and enable pin are just > > > left on all the time, > > > > Correct, on my development board the SN65DSI86 is on all the time, I > > can't control that. > > > > > but plausibly someone might want to build a > > > system to use HPD and also have the enable pin and/or regulators > > > controlled by this driver, right? > > > > True. DRM doesn't make this very easy, as, as far as I can tell, there's > > no standard infrastructure for userspace to register an interest in HPD > > that could be notified to bridges. I think it should be fixable, but > > it's out of scope for this series :-) Should I still add a > > pm_runtime_get_sync() at probe time, or leave this to be addressed by > > someone who will need to implement power control ? > > IMO if you've detected you're running in DP mode you should add the > pm_runtime_get_sync() in probe to keep it powered all the time and > that seems the simplest. Technically I guess that's not really > required since you're polling and you could power off between polls, > but then you'd have to re-init a bunch of your state each time you > polled too. If you ever transitioned to using an IRQ for HPD then > you'd have to keep it always powered anyway. Hrm ... that's precisely what I've done. It's not IRQ based HPD... So would you like to see something like this during ti_sn_bridge_probe()? /* The device must remain powered up for HPD to be supported. */ if (!pdata->no_hpd) pm_runtime_get_sync(pdata->dev); -- Regards Kieran > > -Doug
Hi, On Wed, Feb 23, 2022 at 9:43 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi All, > > I'm working to respin the remainder of these patches, now that I have > IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used > for Renesas R-Car boards. > > Quoting Doug Anderson (2021-06-24 00:51:12) > > Hi, > > > > On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > > > > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > > > pdata->bridge.of_node = client->dev.of_node; > > > > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > > > > > > > Checking for "no_hpd" here is not the right test IIUC. You want to > > > > check for eDP vs. DP (AKA whether a panel is downstream of you or a > > > > connector). Specifically if downstream of you is a panel then (I > > > > believe) HPD won't assert until you turn on the panel and you won't > > > > turn on the panel (which happens in pre_enable, right?) until HPD > > > > fires, so you've got a chicken-and-egg problem. If downstream of you > > > > is a connector, though, then by definition HPD has to just work > > > > without pre_enable running so then you're OK. > > > > > > Agreed. It's even more true now that your rework has landed, as in the > > > eDP case EDID is handled by the panel driver. I'll rework this. > > > > > > Should I also condition setting HPD_DISABLE to the presence of a panel > > > then ? I could drop of_property_read_bool() and set > > > > > > pdata->no_hpd = !!panel; > > > > > > > I guess then you'd need to figure out what to do if someone wants to > > > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > > > > then? Or you could just punt not support this case until someone needs > > > > it. > > > > > > I think I'll stop short of saving the world this time, yes :-) We'll see > > > what to do when this case arises. > > > > How about as a compromise you still call of_property_read_bool() but > > add some type of warning in the logs if someone didn't set "no-hpd" > > but they have a panel? > > > Would a mix of the two work well? > > What about: > > pdata->no_hpd = of_property_read_bool(np, "no-hpd"); > if (panel && !pdata->no_hpd) { > DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n"); > pdata->no_hpd = true; > } > > That leaves it still optional to DP connectors, but enforced on eDP? Yeah, that's fine with me. Nits would be to use "warn" instead of "error" since this isn't fatal and use the non-SHOUTING versions of the prints since the SHOUTING versions are deprecated. > > > > > + | DRM_BRIDGE_OP_EDID; > > > > > > > > IMO somewhere in here if HPD is being used like this you should throw > > > > in a call to pm_runtime_get_sync(). I guess in your solution the > > > > regulators (for the bridge, not the panel) and enable pin are just > > > > left on all the time, > > > > > > Correct, on my development board the SN65DSI86 is on all the time, I > > > can't control that. > > > > > > > but plausibly someone might want to build a > > > > system to use HPD and also have the enable pin and/or regulators > > > > controlled by this driver, right? > > > > > > True. DRM doesn't make this very easy, as, as far as I can tell, there's > > > no standard infrastructure for userspace to register an interest in HPD > > > that could be notified to bridges. I think it should be fixable, but > > > it's out of scope for this series :-) Should I still add a > > > pm_runtime_get_sync() at probe time, or leave this to be addressed by > > > someone who will need to implement power control ? > > > > IMO if you've detected you're running in DP mode you should add the > > pm_runtime_get_sync() in probe to keep it powered all the time and > > that seems the simplest. Technically I guess that's not really > > required since you're polling and you could power off between polls, > > but then you'd have to re-init a bunch of your state each time you > > polled too. If you ever transitioned to using an IRQ for HPD then > > you'd have to keep it always powered anyway. > > > Hrm ... that's precisely what I've done. It's not IRQ based HPD... > > So would you like to see something like this during > ti_sn_bridge_probe()? > > /* The device must remain powered up for HPD to be supported. */ > if (!pdata->no_hpd) > pm_runtime_get_sync(pdata->dev); Yeah, seems reasonable. Probably you'd want to add a devm action to put it too? -Doug
Hi Doug, Quoting Doug Anderson (2022-02-23 18:25:13) > Hi, > > On Wed, Feb 23, 2022 at 9:43 AM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Hi All, > > > > I'm working to respin the remainder of these patches, now that I have > > IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used > > for Renesas R-Car boards. > > > > Quoting Doug Anderson (2021-06-24 00:51:12) > > > Hi, > > > > > > On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart > > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, > > > > > > > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > > > > pdata->bridge.of_node = client->dev.of_node; > > > > > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > > > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > > > > > > > > > Checking for "no_hpd" here is not the right test IIUC. You want to > > > > > check for eDP vs. DP (AKA whether a panel is downstream of you or a > > > > > connector). Specifically if downstream of you is a panel then (I > > > > > believe) HPD won't assert until you turn on the panel and you won't > > > > > turn on the panel (which happens in pre_enable, right?) until HPD > > > > > fires, so you've got a chicken-and-egg problem. If downstream of you > > > > > is a connector, though, then by definition HPD has to just work > > > > > without pre_enable running so then you're OK. > > > > > > > > Agreed. It's even more true now that your rework has landed, as in the > > > > eDP case EDID is handled by the panel driver. I'll rework this. > > > > > > > > Should I also condition setting HPD_DISABLE to the presence of a panel > > > > then ? I could drop of_property_read_bool() and set > > > > > > > > pdata->no_hpd = !!panel; > > > > > > > > > I guess then you'd need to figure out what to do if someone wants to > > > > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > > > > > then? Or you could just punt not support this case until someone needs > > > > > it. > > > > > > > > I think I'll stop short of saving the world this time, yes :-) We'll see > > > > what to do when this case arises. > > > > > > How about as a compromise you still call of_property_read_bool() but > > > add some type of warning in the logs if someone didn't set "no-hpd" > > > but they have a panel? > > > > > > Would a mix of the two work well? > > > > What about: > > > > pdata->no_hpd = of_property_read_bool(np, "no-hpd"); > > if (panel && !pdata->no_hpd) { > > DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n"); > > pdata->no_hpd = true; > > } > > > > That leaves it still optional to DP connectors, but enforced on eDP? > > Yeah, that's fine with me. Nits would be to use "warn" instead of > "error" since this isn't fatal and use the non-SHOUTING versions of > the prints since the SHOUTING versions are deprecated. Could you clarify this please? The whole driver uses DRM_ERROR style. Is there a new debug macro somewhere? > > > > > > > > + | DRM_BRIDGE_OP_EDID; > > > > > > > > > > IMO somewhere in here if HPD is being used like this you should throw > > > > > in a call to pm_runtime_get_sync(). I guess in your solution the > > > > > regulators (for the bridge, not the panel) and enable pin are just > > > > > left on all the time, > > > > > > > > Correct, on my development board the SN65DSI86 is on all the time, I > > > > can't control that. > > > > > > > > > but plausibly someone might want to build a > > > > > system to use HPD and also have the enable pin and/or regulators > > > > > controlled by this driver, right? > > > > > > > > True. DRM doesn't make this very easy, as, as far as I can tell, there's > > > > no standard infrastructure for userspace to register an interest in HPD > > > > that could be notified to bridges. I think it should be fixable, but > > > > it's out of scope for this series :-) Should I still add a > > > > pm_runtime_get_sync() at probe time, or leave this to be addressed by > > > > someone who will need to implement power control ? > > > > > > IMO if you've detected you're running in DP mode you should add the > > > pm_runtime_get_sync() in probe to keep it powered all the time and > > > that seems the simplest. Technically I guess that's not really > > > required since you're polling and you could power off between polls, > > > but then you'd have to re-init a bunch of your state each time you > > > polled too. If you ever transitioned to using an IRQ for HPD then > > > you'd have to keep it always powered anyway. > > > > > > Hrm ... that's precisely what I've done. It's not IRQ based HPD... > > > > So would you like to see something like this during > > ti_sn_bridge_probe()? > > > > /* The device must remain powered up for HPD to be supported. */ > > if (!pdata->no_hpd) > > pm_runtime_get_sync(pdata->dev); > > Yeah, seems reasonable. Probably you'd want to add a devm action to put it too? Ok, looking at this now - then I should be able to get these updated patches out. -- Thanks. Kieran > > -Doug
Hi, On Fri, Mar 4, 2022 at 7:46 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > > > What about: > > > > > > pdata->no_hpd = of_property_read_bool(np, "no-hpd"); > > > if (panel && !pdata->no_hpd) { > > > DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n"); > > > pdata->no_hpd = true; > > > } > > > > > > That leaves it still optional to DP connectors, but enforced on eDP? > > > > Yeah, that's fine with me. Nits would be to use "warn" instead of > > "error" since this isn't fatal and use the non-SHOUTING versions of > > the prints since the SHOUTING versions are deprecated. > > Could you clarify this please? The whole driver uses DRM_ERROR style. Is > there a new debug macro somewhere? Mostly looking at commit 306589856399 ("drm/print: Add deprecation notes to DRM_...() functions"), which I added a few months ago. Despite the fact that I added the documentation, though, I'm not the one driving the transition away from the SHOUTing versions. If you look through history you can see this is driven by more senior DRM people. IMO it's fine to transition slowly to the new non-shouting versions and it doesn't bother me to have some code in a file using the old SHOUTing versions and some using the newer functions. Basically for new code or when we're touching code anyway we do the transition then. That being said, if you want to -Doug
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f792227142a7..72f6362adf44 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -167,6 +167,8 @@ struct ti_sn_bridge { struct gpio_chip gchip; DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS); #endif + + bool no_hpd; }; static const struct regmap_range ti_sn_bridge_volatile_ranges[] = { @@ -862,23 +864,28 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) ti_sn_bridge_set_refclk_freq(pdata); /* - * HPD on this bridge chip is a bit useless. This is an eDP bridge - * so the HPD is an internal signal that's only there to signal that - * the panel is done powering up. ...but the bridge chip debounces - * this signal by between 100 ms and 400 ms (depending on process, - * voltage, and temperate--I measured it at about 200 ms). One + * As this is an eDP bridge, the output will be connected to a fixed + * panel in most systems. HPD is in that case only an internal signal + * to signal that the panel is done powering up. The bridge chip + * debounces this signal by between 100 ms and 400 ms (depending on + * process, voltage, and temperate--I measured it at about 200 ms). One * particular panel asserted HPD 84 ms after it was powered on meaning * that we saw HPD 284 ms after power on. ...but the same panel said * that instead of looking at HPD you could just hardcode a delay of - * 200 ms. We'll assume that the panel driver will have the hardcoded - * delay in its prepare and always disable HPD. + * 200 ms. HPD is thus a bit useless. For this type of use cases, we'll + * assume that the panel driver will have the hardcoded delay in its + * prepare and always disable HPD. * - * If HPD somehow makes sense on some future panel we'll have to - * change this to be conditional on someone specifying that HPD should - * be used. + * However, on some systems, the output is connected to a DisplayPort + * connector. HPD is needed in such cases. To accommodate both use + * cases, enable HPD only when requested. */ - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, - HPD_DISABLE); + if (pdata->no_hpd) + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, + HPD_DISABLE, HPD_DISABLE); + else + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, + HPD_DISABLE, 0); } static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) pm_runtime_put_sync(pdata->dev); } +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge *bridge) +{ + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); + int val; + + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, &val); + return val ? connector_status_connected : connector_status_disconnected; +} + static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge, struct drm_connector *connector) { @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .enable = ti_sn_bridge_enable, .disable = ti_sn_bridge_disable, .post_disable = ti_sn_bridge_post_disable, + .detect = ti_sn_bridge_detect, .get_edid = ti_sn_bridge_get_edid, }; @@ -1327,6 +1344,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, return ret; } + pdata->no_hpd = of_property_read_bool(pdata->dev->of_node, "no-hpd"); + ti_sn_bridge_parse_lanes(pdata, client->dev.of_node); ret = ti_sn_bridge_parse_regulators(pdata); @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = client->dev.of_node; - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) + | DRM_BRIDGE_OP_EDID; pdata->bridge.type = pdata->panel ? DRM_MODE_CONNECTOR_eDP : DRM_MODE_CONNECTOR_DisplayPort;
When the SN65DSI86 is used in DisplayPort mode, its output is likely routed to a DisplayPort connector, which can benefit from hotplug detection. Support it in such cases, with polling mode only for now. The implementation is limited to the bridge operations, as the connector operations are legacy and new users should use DRM_BRIDGE_ATTACH_NO_CONNECTOR. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++++++++++++++++++-------- 1 file changed, 33 insertions(+), 13 deletions(-)