On 21.05.2019 13:34, Tomi Valkeinen wrote:
> 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.
aah, I forgot about this pattern :)
>
>>> +
>>> + 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.
.detect can be also called multiple times.
>
> 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.
If I remember correctly without hpd userspace 'informs' driver that sink
is connected (via status sysfs property), in such case
.fill_modes/.get_modes is called on change.
>
> 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.
My approach and experience suggest that detect, should be rather
lightweight and should not modify state, I am not even sure if it is
called at all on forced connector.
Regards
Andrzej
>
>> 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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
