| Message ID | 20201210134648.272857-6-maxime@cerno.tech |
|---|---|
| State | New |
| Headers | show |
| Series | drm/vc4: hdmi: Add CEC support for the BCM2711 | expand |
Hi Maxime & Dom On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > From: Dom Cobley <popcornmix@gmail.com> > > Currently we call cec_phys_addr_invalidate on a hotplug deassert. > That may be due to a TV power cycling, or an AVR being switched > on (and switching edid). > > This makes CEC unusable since our controller wouldn't have a physical > address anymore. > > Set it back up again on the hotplug assert. > > Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 28b78ea885ea..eff3bac562c6 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -136,20 +136,29 @@ static enum drm_connector_status > vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > { > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > + bool connected = false; > > if (vc4_hdmi->hpd_gpio) { > if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ > vc4_hdmi->hpd_active_low) > - return connector_status_connected; > - cec_phys_addr_invalidate(vc4_hdmi->cec_adap); > - return connector_status_disconnected; > - } > - > - if (drm_probe_ddc(vc4_hdmi->ddc)) > - return connector_status_connected; > - > + connected = true; > + } else if (drm_probe_ddc(vc4_hdmi->ddc)) > + connected = true; > if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) This needs to become an "else if(...". It used to be that all the other paths would return, so were mutually exclusive to this. Now they set a thing and keep going we need to avoid reading the register should there be a HPD gpio or the ddc probe succeeds. Memory says that otherwise Pi3 always reports connected. I fixed this in a downstream patch already - https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453a0d3e98c Otherwise fine. Dave > + connected = true; > + if (connected) { > + if (connector->status != connector_status_connected) { > + struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc); > + > + if (edid) { > + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); > + vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); > + drm_connector_update_edid_property(connector, edid); > + kfree(edid); > + } > + } > return connector_status_connected; > + } > cec_phys_addr_invalidate(vc4_hdmi->cec_adap); > return connector_status_disconnected; > } > -- > 2.28.0 >
On Fri, 18 Dec 2020 at 14:21, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Maxime & Dom > > On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > > > From: Dom Cobley <popcornmix@gmail.com> > > > > Currently we call cec_phys_addr_invalidate on a hotplug deassert. > > That may be due to a TV power cycling, or an AVR being switched > > on (and switching edid). > > > > This makes CEC unusable since our controller wouldn't have a physical > > address anymore. > > > > Set it back up again on the hotplug assert. > > > > Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") > > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- > > 1 file changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index 28b78ea885ea..eff3bac562c6 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -136,20 +136,29 @@ static enum drm_connector_status > > vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > > { > > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > + bool connected = false; > > > > if (vc4_hdmi->hpd_gpio) { > > if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ > > vc4_hdmi->hpd_active_low) > > - return connector_status_connected; > > - cec_phys_addr_invalidate(vc4_hdmi->cec_adap); > > - return connector_status_disconnected; > > - } > > - > > - if (drm_probe_ddc(vc4_hdmi->ddc)) > > - return connector_status_connected; > > - > > + connected = true; > > + } else if (drm_probe_ddc(vc4_hdmi->ddc)) > > + connected = true; > > if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) > > This needs to become an "else if(...". > It used to be that all the other paths would return, so were mutually > exclusive to this. Now they set a thing and keep going we need to > avoid reading the register should there be a HPD gpio or the ddc probe > succeeds. > Memory says that otherwise Pi3 always reports connected. > > I fixed this in a downstream patch already - > https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453a0d3e98c > > Otherwise fine. > > Dave > > > + connected = true; > > + if (connected) { > > + if (connector->status != connector_status_connected) { > > + struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc); > > + > > + if (edid) { > > + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); > > + vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); > > + drm_connector_update_edid_property(connector, edid); Actually looking at this again in the context of the other changes, do we need to call drm_connector_update_edid_property() here? We've just called drm_get_edid() to get the edid, and that calls drm_connector_update_edid_property() as well [1] Updating vc4_hdmi->encoder.hdmi_monitor may be necessary. It's otherwise done in vc4_hdmi_connector_get_modes, which I sort of expect to be called almost immediately by the framework when connector_detect returns "connected". I haven't checked if that is guaranteed though. vc4_hdmi_connector_get_modes also includes a manual call to drm_connector_update_edid_property after having just called drm_get_edid, so that one feels redundant too. Dave [1] https://elixir.bootlin.com/linux/v5.10/source/drivers/gpu/drm/drm_edid.c#L2059 > > + kfree(edid); > > + } > > + } > > return connector_status_connected; > > + } > > cec_phys_addr_invalidate(vc4_hdmi->cec_adap); > > return connector_status_disconnected; > > } > > -- > > 2.28.0 > >
Hi Dave, Thanks for your review On Fri, Dec 18, 2020 at 02:45:54PM +0000, Dave Stevenson wrote: > On Fri, 18 Dec 2020 at 14:21, Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Maxime & Dom > > > > On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > From: Dom Cobley <popcornmix@gmail.com> > > > > > > Currently we call cec_phys_addr_invalidate on a hotplug deassert. > > > That may be due to a TV power cycling, or an AVR being switched > > > on (and switching edid). > > > > > > This makes CEC unusable since our controller wouldn't have a physical > > > address anymore. > > > > > > Set it back up again on the hotplug assert. > > > > > > Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support") > > > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++-------- > > > 1 file changed, 17 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index 28b78ea885ea..eff3bac562c6 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -136,20 +136,29 @@ static enum drm_connector_status > > > vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > > > { > > > struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > > + bool connected = false; > > > > > > if (vc4_hdmi->hpd_gpio) { > > > if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ > > > vc4_hdmi->hpd_active_low) > > > - return connector_status_connected; > > > - cec_phys_addr_invalidate(vc4_hdmi->cec_adap); > > > - return connector_status_disconnected; > > > - } > > > - > > > - if (drm_probe_ddc(vc4_hdmi->ddc)) > > > - return connector_status_connected; > > > - > > > + connected = true; > > > + } else if (drm_probe_ddc(vc4_hdmi->ddc)) > > > + connected = true; > > > if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) > > > > This needs to become an "else if(...". > > It used to be that all the other paths would return, so were mutually > > exclusive to this. Now they set a thing and keep going we need to > > avoid reading the register should there be a HPD gpio or the ddc probe > > succeeds. > > Memory says that otherwise Pi3 always reports connected. > > > > I fixed this in a downstream patch already - > > https://github.com/raspberrypi/linux/commit/d345caec1e9b2317b9cd7eb5b92ae453a0d3e98c > > > > Otherwise fine. > > > > Dave > > > > > + connected = true; > > > + if (connected) { > > > + if (connector->status != connector_status_connected) { > > > + struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc); > > > + > > > + if (edid) { > > > + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); > > > + vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); > > > + drm_connector_update_edid_property(connector, edid); > > Actually looking at this again in the context of the other changes, do > we need to call drm_connector_update_edid_property() here? > > We've just called drm_get_edid() to get the edid, and that calls > drm_connector_update_edid_property() as well [1] Yeah, you're right I'll drop it > Updating vc4_hdmi->encoder.hdmi_monitor may be necessary. It's > otherwise done in vc4_hdmi_connector_get_modes, which I sort of expect > to be called almost immediately by the framework when connector_detect > returns "connected". I haven't checked if that is guaranteed though. > > vc4_hdmi_connector_get_modes also includes a manual call to > drm_connector_update_edid_property after having just called > drm_get_edid, so that one feels redundant too. .get_modes is called in drm_helper_probe_single_connector_modes, which is usually the helper set in .fill_modes. .fill_modes seems to only be called when either DRM_IOCTL_MODE_GETCONNECTOR is called, or when the connector status is forced through sysfs, so it doesn't look like it's done automatically. I'm not sure we need to set hdmi_monitor though, it's only used to configure the display related side, and that can't happen without get_modes being called. Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 28b78ea885ea..eff3bac562c6 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -136,20 +136,29 @@ static enum drm_connector_status vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) { struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); + bool connected = false; if (vc4_hdmi->hpd_gpio) { if (gpio_get_value_cansleep(vc4_hdmi->hpd_gpio) ^ vc4_hdmi->hpd_active_low) - return connector_status_connected; - cec_phys_addr_invalidate(vc4_hdmi->cec_adap); - return connector_status_disconnected; - } - - if (drm_probe_ddc(vc4_hdmi->ddc)) - return connector_status_connected; - + connected = true; + } else if (drm_probe_ddc(vc4_hdmi->ddc)) + connected = true; if (HDMI_READ(HDMI_HOTPLUG) & VC4_HDMI_HOTPLUG_CONNECTED) + connected = true; + if (connected) { + if (connector->status != connector_status_connected) { + struct edid *edid = drm_get_edid(connector, vc4_hdmi->ddc); + + if (edid) { + cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); + vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); + drm_connector_update_edid_property(connector, edid); + kfree(edid); + } + } return connector_status_connected; + } cec_phys_addr_invalidate(vc4_hdmi->cec_adap); return connector_status_disconnected; }