On 21/05/2019 10:07, Andrzej Hajda wrote:
@@ -1214,19 +1234,43 @@ static int tc_connector_get_modes(struct drm_connector *connector)
return count;
}
-static void tc_connector_set_polling(struct tc_data *tc,
- struct drm_connector *connector)
-{
- /* TODO: add support for HPD */
- connector->polled = DRM_CONNECTOR_POLL_CONNECT |
- DRM_CONNECTOR_POLL_DISCONNECT;
-}
-
static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
.get_modes = tc_connector_get_modes,
};
+static enum drm_connector_status tc_connector_detect(struct drm_connector *connector,
+ bool force)
+{
+ struct tc_data *tc = connector_to_tc(connector);
+ bool conn;
+ u32 val;
+ int ret;
unused var
Needed for tc_write/read... =( Cleaning these up will be the next step.
+
+ if (tc->hpd_pin < 0) {
+ if (!tc->panel)
+ return connector_status_unknown;
+
+ conn = true;
+ } else {
+ tc_read(GPIOI, &val);
+
+ conn = val & BIT(tc->hpd_pin);
+ }
+
+ if (force && conn)
+ tc_get_display_props(tc);
Why do you call tc_get_display_props here? It is called already in .enable.
If you need it for get_modes you can call it there. Here it looks unrelated.
Yes, it's needed for get_modes. Or more specifically, for tc_mode_valid. I agree it
doesn't quite feel right, but I wouldn't say it's unrelated, or that this is a wrong place.
Afaics, we need tc_get_display_props in bridge_enable, for the case where we don't have
hpd. We could call tc_get_display_props in get_modes, but I don't know if we always get a
get_modes call. Or maybe we get multiple get_modes calls, and we do unnecessary
tc_get_display_props calls.
Now that I wrote the above, it makes me wonder whether the get_modes works in the current
patches if we don't have hpd...
We could cache tc_get_display_props results, too, but I'm not sure when to clear the
cache, especially if we don't have hpd.
DisplayPort spec talks about doing the display-props reading and EDID reading when
handling HPD.
I think it would be best to change the code so that we read display props and EDID in HPD,
but so that we also can read them later (when needed, probably bridge enable and
get_modes) if we haven't done the reads already. I've had this in mind since I started the
series, but as it didn't feel like a simple change, I left it for later.
Removing the call from here should also simplify function flow:
if (tc->hpd_pin < 0)
return tc->panel ? connector_status_connected :
connector_status_disconnected;
tc_read(GPIOI, &val);
return (val & BIT(tc->hpd_pin))? ? connector_status_connected :
connector_status_disconnected;
+
+ if (conn)
+ return connector_status_connected;
+ else
+ return connector_status_disconnected;
+
+err:
unused label/code?
Needed for tc_write/read too.
@@ -1249,6 +1293,15 @@ static int tc_bridge_attach(struct drm_bridge *bridge)
if (ret)
return ret;
+ /* Don't poll if don't have HPD connected */
+ if (tc->hpd_pin >= 0) {
+ if (tc->have_irq)
+ tc->connector.polled = DRM_CONNECTOR_POLL_HPD;
+ else
+ tc->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
+ DRM_CONNECTOR_POLL_DISCONNECT;
Bikeshedding: wouldn't be more clear to use ?: operator?
Depends on the reader, I guess. I like ?: when the parameters are relatively simple (say,
a single variable). Here it's a bit so-and-so with the second case's bitwise-or.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID:
0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
