[PATCHv3 22/23] drm/bridge: tc358767: add IRQ and HPD support

[Andrzej Hajda]

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
>