Re: [PATCH 4/9] drm/connector: Add support for out-of-band hotplug notification (v2)

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/5/21 11:50 AM, Heikki Krogerus wrote:
> On Tue, May 04, 2021 at 05:35:49PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 5/4/21 5:10 PM, Heikki Krogerus wrote:
>>>> +/**
>>>> + * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to connector
>>>> + * @connector: connector to report the event on
>>>> + * @data: data related to the event
>>>> + *
>>>> + * On some hardware a hotplug event notification may come from outside the display
>>>> + * driver / device. An example of this is some USB Type-C setups where the hardware
>>>> + * muxes the DisplayPort data and aux-lines but does not pass the altmode HPD
>>>> + * status bit to the GPU's DP HPD pin.
>>>> + *
>>>> + * This function can be used to report these out-of-band events after obtaining
>>>> + * a drm_connector reference through calling drm_connector_find_by_fwnode().
>>>> + */
>>>> +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
>>>> +				     struct drm_connector_oob_hotplug_event_data *data)
>>>> +{
>>>> +	struct drm_connector *connector;
>>>> +
>>>> +	connector = drm_connector_find_by_fwnode(connector_fwnode);
>>>> +	if (IS_ERR(connector))
>>>> +		return;
>>>> +
>>>> +	if (connector->funcs->oob_hotplug_event)
>>>> +		connector->funcs->oob_hotplug_event(connector, data);
>>>> +
>>>> +	drm_connector_put(connector);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_connector_oob_hotplug_event);
>>>
>>> So it does looks like the "data" parameter is not needed at all:
>>
>> Well Imre did indicate that having the number of lanes is useful, so
>> for the next version I'll drop the orientation but I plan to keep
>> the number of lanes if that is ok with you.
>>
>> Not having passing along this info was one of the reasons why my
>> previous attempt at this was nacked, so dropping it all together
>> feels wrong.
> 
> If you need to pass any information to the function, then you need to
> pass all the information that we have. Don't start with abstraction.
> First create a dedicated API, and then, only if we really have another
> user for it, we can add an abstraction layer that both users can use.
> All cases are going to be different. We don't know how the abstraction
> / generalisation should look like before we have at least two real
> users (ideally more than two, actually). Right now we can not even say
> for sure that generalising the API is even possible.
> 
> I would not make a huge deal out of this unless I wasn't myself being
> told by guys like Greg KH in the past to drop my attempts to
> "generalize" things from the beginning when I only had a single user.
> By doing so you'll not only force all ends, the source of the data
> (the typec drivers in this case) as well as the consumer (i915), to be
> always changed together, it will also confuse things. We are not
> always going to be able to tell the lane count for example like we can
> with USB Type-C, so i915 can't really rely on that information.
> 
> Right now we also don't know what exact details i915 (or what ever GPU
> driver) needs. We can only say for sure that some details are going to
> be needed. Trying to guess and cherry-pick the details now does not
> makes sense because of that reason too.
> 
> So just make this API USB Type-C DP Alt Mode specific API first, and
> supply it everything we have.

Hmm, ok I'll just drop the data argument all together for now (as you
already suggested); and then we can see what is best once an actual user
for the info shows up.

Regards,

Hans