On 20.08.2019 00:45, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Mon, Aug 19, 2019 at 10:38:35AM +0200, Andrzej Hajda wrote:
>> On 14.08.2019 14:40, Daniel Vetter wrote:
>>> On Wed, Aug 14, 2019 at 01:04:03PM +0300, Laurent Pinchart wrote:
>>>> On Wed, Aug 14, 2019 at 08:23:12AM +0200, Andrzej Hajda wrote:
>>>>> On 11.08.2019 00:43, Laurent Pinchart wrote:
>>>>>> On Fri, Aug 09, 2019 at 01:55:53PM +0200, Andrzej Hajda wrote:
>>>>>>> On 08.08.2019 21:32, Laurent Pinchart wrote:
>>>>>>>> On Tue, Jul 16, 2019 at 03:57:21PM +0200, Andrzej Hajda wrote:
>>>>>>>>> On 16.07.2019 11:00, Daniel Vetter wrote:
>>>>>>>>>> On Fri, Jul 12, 2019 at 11:01:38AM +0200, Andrzej Hajda wrote:
>>>>>>>>>>> On 11.07.2019 17:50, Daniel Vetter wrote:
>>>>>>>>>>>> On Thu, Jul 11, 2019 at 05:12:26PM +0200, Andrzej Hajda wrote:
>>>>>>>>>>>>> On 11.07.2019 15:18, Daniel Vetter wrote:
>>>>>>>>>>>>>> On Thu, Jul 11, 2019 at 02:41:01PM +0200, Andrzej Hajda wrote:
>>>>>>>>>>>>>>> On 11.07.2019 09:35, Daniel Vetter wrote:
>>>>>>>>>>>>>>>> On Wed, Jul 10, 2019 at 02:12:14PM +0200, Andrzej Hajda wrote:
>>>>>>>>>>>>>>>>> Hi Laurent,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I like the approach, current practice when almost every bridge should
>>>>>>>>>>>>>>>>> optionally implement connector, or alternatively downstream bridge or
>>>>>>>>>>>>>>>>> panel is very painful.
>>>>>>>>>>>>>>>> Yeah I think this looks mostly reasonable. Some api design comments on top
>>>>>>>>>>>>>>>> of Andrzej', with the fair warning that I didn't bother to read up on how
>>>>>>>>>>>>>>>> it's all used in the end. I probably should go and do that, at least to
>>>>>>>>>>>>>>>> get a feeling for what your hpd_cb usually does.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> More comments inlined.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 07.07.2019 20:18, Laurent Pinchart wrote:
>>>>>>>>>>>>>>>>>> To support implementation of DRM connectors on top of DRM bridges
>>>>>>>>>>>>>>>>>> instead of by bridges, the drm_bridge needs to expose new operations and
>>>>>>>>>>>>>>>>>> data:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> - Output detection, hot-plug notification, mode retrieval and EDID
>>>>>>>>>>>>>>>>>> retrieval operations
>>>>>>>>>>>>>>>>>> - Bitmask of supported operations
>>>>>>>>>>>>>>>>> Why do we need these bitmask at all? Why cannot we rely on presence of
>>>>>>>>>>>>>>>>> operation's callback?
>>>>>>>>>>>>>>>> Yeah also not a huge fan of these bitmasks. Smells like
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> DRIVER_GEM|DRIVER_MODESET, and I personally really hate those. Easy to
>>>>>>>>>>>>>>>> add, generally good excuse to not have to think through the design between
>>>>>>>>>>>>>>>> different parts of drivers - "just" add another flag.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> - Bridge output type
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Add and document these.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Three new bridge helper functions are also added to handle hot plug
>>>>>>>>>>>>>>>>>> notification in a way that is as transparent as possible for the
>>>>>>>>>>>>>>>>>> bridges.
>>>>>>>>>>>>>>>>> Documentation of new opses does not explain how it should cooperate with
>>>>>>>>>>>>>>>>> bridge chaining, I suppose they should be chained explicitly, am I
>>>>>>>>>>>>>>>>> right? More comments about it later.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>> drivers/gpu/drm/drm_bridge.c | 92 +++++++++++++++++++
>>>>>>>>>>>>>>>>>> include/drm/drm_bridge.h | 170 ++++++++++++++++++++++++++++++++++-
>>>>>>>>>>>>>>>>>> 2 files changed, 261 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>>>>>>>>>>>>>>> index 519577f363e3..3c2a255df7af 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>>>>>>>>>>>>>> @@ -70,6 +70,8 @@ static LIST_HEAD(bridge_list);
>>>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>>>> void drm_bridge_add(struct drm_bridge *bridge)
>>>>>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>>>>> + mutex_init(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> mutex_lock(&bridge_lock);
>>>>>>>>>>>>>>>>>> list_add_tail(&bridge->list, &bridge_list);
>>>>>>>>>>>>>>>>>> mutex_unlock(&bridge_lock);
>>>>>>>>>>>>>>>>>> @@ -86,6 +88,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>>>>>>>>>>>>>>>>>> mutex_lock(&bridge_lock);
>>>>>>>>>>>>>>>>>> list_del_init(&bridge->list);
>>>>>>>>>>>>>>>>>> mutex_unlock(&bridge_lock);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + mutex_destroy(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> EXPORT_SYMBOL(drm_bridge_remove);
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> @@ -463,6 +467,94 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> EXPORT_SYMBOL(drm_atomic_bridge_enable);
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +/**
>>>>>>>>>>>>>>>>>> + * drm_bridge_hpd_enable - enable hot plug detection for the bridge
>>>>>>>>>>>>>>>>>> + * @bridge: bridge control structure
>>>>>>>>>>>>>>>>>> + * @cb: hot-plug detection callback
>>>>>>>>>>>>>>>>>> + * @data: data to be passed to the hot-plug detection callback
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Call &drm_bridge_funcs.hpd_enable and register the given @cb and @data as
>>>>>>>>>>>>>>>>>> + * hot plug notification callback. From now on the @cb will be called with
>>>>>>>>>>>>>>>>>> + * @data when an output status change is detected by the bridge, until hot plug
>>>>>>>>>>>>>>>>>> + * notification gets disabled with drm_bridge_hpd_disable().
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in
>>>>>>>>>>>>>>>>>> + * bridge->ops. This function shall not be called when the flag is not set.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Only one hot plug detection callback can be registered at a time, it is an
>>>>>>>>>>>>>>>>>> + * error to call this function when hot plug detection is already enabled for
>>>>>>>>>>>>>>>>>> + * the bridge.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>> To simplify architecture maybe would be better to enable hpd just on
>>>>>>>>>>>>>>>>> bridge attach:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> bridge->hpd_cb = cb;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> bridge->hpd_data = data;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ret = drm_bridge_attach(...);
>>>>>>>>>>>>>>>> Yeah I like this more. The other problem here is, what if you need more
>>>>>>>>>>>>>>>> than 1 callback registers on the same bridge hdp signal?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This way we could avoid adding new callbacks hpd_(enable|disable)
>>>>>>>>>>>>>>>>> without big sacrifices.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> One more thing: HPD in DisplayPort/HDMI beside signalling plug/unplug,
>>>>>>>>>>>>>>>>> notifies about sink status change, how it translates to this cb?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +void drm_bridge_hpd_enable(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> + void (*cb)(void *data,
>>>>>>>>>>>>>>>>>> + enum drm_connector_status status),
>>>>>>>>>>>>>>>>>> + void *data)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + if (!bridge || !bridge->funcs->hpd_enable)
>>>>>>>>>>>>>>>>>> + return;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + mutex_lock(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (WARN(bridge->hpd_cb, "Hot plug detection already enabled\n"))
>>>>>>>>>>>>>>>>>> + goto unlock;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + bridge->hpd_cb = cb;
>>>>>>>>>>>>>>>>>> + bridge->hpd_data = data;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + bridge->funcs->hpd_enable(bridge);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +unlock:
>>>>>>>>>>>>>>>>>> + mutex_unlock(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(drm_bridge_hpd_enable);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +/**
>>>>>>>>>>>>>>>>>> + * drm_bridge_hpd_disable - disable hot plug detection for the bridge
>>>>>>>>>>>>>>>>>> + * @bridge: bridge control structure
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Call &drm_bridge_funcs.hpd_disable and unregister the hot plug detection
>>>>>>>>>>>>>>>>>> + * callback previously registered with drm_bridge_hpd_enable(). Once this
>>>>>>>>>>>>>>>>>> + * function returns the callback will not be called by the bridge when an
>>>>>>>>>>>>>>>>>> + * output status change occurs.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Hot plug detection is supported only if the DRM_BRIDGE_OP_HPD flag is set in
>>>>>>>>>>>>>>>>>> + * bridge->ops. This function shall not be called when the flag is not set.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> +void drm_bridge_hpd_disable(struct drm_bridge *bridge)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + if (!bridge || !bridge->funcs->hpd_disable)
>>>>>>>>>>>>>>>>>> + return;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + mutex_lock(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> + bridge->funcs->hpd_disable(bridge);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + bridge->hpd_cb = NULL;
>>>>>>>>>>>>>>>>>> + bridge->hpd_data = NULL;
>>>>>>>>>>>>>>>>>> + mutex_unlock(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(drm_bridge_hpd_disable);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +/**
>>>>>>>>>>>>>>>>>> + * drm_bridge_hpd_notify - notify hot plug detection events
>>>>>>>>>>>>>>>>>> + * @bridge: bridge control structure
>>>>>>>>>>>>>>>>>> + * @status: output connection status
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Bridge drivers shall call this function to report hot plug events when they
>>>>>>>>>>>>>>>>>> + * detect a change in the output status, when hot plug detection has been
>>>>>>>>>>>>>>>>>> + * enabled by the &drm_bridge_funcs.hpd_enable callback.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This function shall be called in a context that can sleep.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> +void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> + enum drm_connector_status status)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + mutex_lock(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> + if (bridge->hpd_cb)
>>>>>>>>>>>>>>>>>> + bridge->hpd_cb(bridge->hpd_data, status);
>>>>>>>>>>>>>>>> So this isn't quite what I had in mind. Instead something like this:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> /* iterates over all bridges in the chain containing @bridge */
>>>>>>>>>>>>>>>> for_each_bridge(tmp_bridge, bridge) {
>>>>>>>>>>>>>>>> if (tmp_bridge == bridge)
>>>>>>>>>>>>>>>> continue;
>>>>>>>>>>>>>>>> if (bridge->hpd_notify);
>>>>>>>>>>>>>>>> bridge->hpd_notify(tmp_bridge, bridge, status);
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> encoder = encoder_for_bridge(bridge);
>>>>>>>>>>>>>>>> if (encoder->helper_private->bridge_hpd_notify)
>>>>>>>>>>>>>>>> encoder->helper_private->bridge_hpd_notify(encoder, bridge, status);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> dev = bridge->dev
>>>>>>>>>>>>>>>> if (dev->mode_config.helper_private->bridge_hpd_notify)
>>>>>>>>>>>>>>>> dev->mode_config.helper_private->bridge_hpd_notify(dev, bridge, status)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> No register callback needed, no locking needed, everyone gets exactly the
>>>>>>>>>>>>>>>> hpd they want/need.
>>>>>>>>>>>>>>> As I understand you want to notify every member of the pipeline.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I think it should be enough to notify only the source, and then source
>>>>>>>>>>>>>>> should decide if/when the hpd should be propagated upstream.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It looks more generic for me.
>>>>>>>>>>>>>> I'm not parsing ... do you think my idea is more generic and useful, or
>>>>>>>>>>>>>> the one from Laurent? Kinda confused here.
>>>>>>>>>>>>> Regarding general idea:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Laurent's approach is to notify only consumer, I guess usually video
>>>>>>>>>>>>> source.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2. Your is to notify all other bridges and encoder.
>>>>>>>>>>>>>
>>>>>>>>>>>>> And I prefer 1st approach, why:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - the source can decide if/when and to who propagate the signal,
>>>>>>>>>>>>>
>>>>>>>>>>>>> - is more generic, for example if bridge send signal to two
>>>>>>>>>>>>> monitors/panels, it can delay hpd propagation till both sinks are present,
>>>>>>>>>>>> With Laurent's approach the bridge cannot send the hpd to more than one
>>>>>>>>>>>> consumer. There's only 1 callback. So you're example doesn't work.
>>>>>>>>>>> If there will be two consumers, there will be two bridge attachments,
>>>>>>>>>>> thus there will be two notifications, it should work.
>>>>>>>>>> 2 consumers, 1 producer. There's only _one_ callback in the producer. The
>>>>>>>>>> callback is registered on the produce bridge, not on the consumer bridge
>>>>>>>>>> (or I'm totallly misreading what Laurent does here).
>>>>>>>>> I have assumed that if devices exposes two hardware sink interfaces it
>>>>>>>>> will expose two separate bridges - of course it will not work with
>>>>>>>>> "bridge chaining" thing, but this is a different story.
>>>>>>>> Daniel is right that the current implementation only allows one
>>>>>>>> consumer. This is however not a limitation of the API, but of its
>>>>>>>> implementation, as I only needed a single consumer. The helpers in this
>>>>>>>> series ensure that neither the consumer nor the producer poke in the
>>>>>>>> drm_bridge structure to call back to the HPD handler:
>>>>>>>>
>>>>>>>> - The consumer calls drm_bridge_hpd_enable() and
>>>>>>>> drm_bridge_hpd_disable(), which could offer a reference-counted
>>>>>>>> behaviour if desired without changes to the consumer.
>>>>>>>>
>>>>>>>> - The producer gets configured by .hpd_enable() and .hpd_disable(),
>>>>>>>> which could also easily accommodate reference-counting in the drm
>>>>>>>> bridge core without changes to the producer.
>>>>>>>>
>>>>>>>> - The producer notifies HPD with drm_bridge_hpd_notify(), which could
>>>>>>>> easily be extended to support multiple consumers without changes to
>>>>>>>> the producer.
>>>>>>>>
>>>>>>>> This is actually my second version of the HPD mechanism. The first
>>>>>>>> version was never posted, poked into drm_bridge, and required the
>>>>>>>> producer to be aware of the callbacks. After discussing this privately
>>>>>>>> with Daniel, I came up with the implementation in this series that,
>>>>>>>> while not supporting multiple consumers now, makes it easy to extend
>>>>>>>> later without minimal effort.
>>>>>>>>
>>>>>>>> Daniel's proposed implementation above looks reasonable to me, provided
>>>>>>>> we can iterate over the bridges in an order that don't depend on the
>>>>>>>> position of the producer in the chain (should be easy to solve by
>>>>>>>> starting at the encoder for instance). It however looks a bit like a
>>>>>>>> midlayer to me :-) That's why I have a similar implementation in the
>>>>>>>> connector-bridge helper, which could be extended to call
>>>>>>>> encoder->helper_private->bridge_hpd_notify() and
>>>>>>>> dev->mode_config.helper_private->bridge_hpd_notify() instead of
>>>>>>>> hardcoding drm_kms_helper_hotplug_event(). Moving the code to
>>>>>>>> drm_bridge_hpd_notify() would on the other hand set the notification
>>>>>>>> sequence towards the encoder and driver in stone. Daniel, do you think
>>>>>>>> that would be better ?
>>>>>>>>
>>>>>>>> I would like to remind everybody that this series isn't the last I will
>>>>>>>> ever submit, and I plan to do more work on drm_bridge and drm_panel. I'm
>>>>>>>> open to suggestions, and can address problems on top of these patches,
>>>>>>>> provided obviously that this series doesn't go in the wrong direction.
>>>>>>>> I'm of course also willing to rework this series, but given the amount
>>>>>>>> of work we have in the drm_bridge realm, I can't fix everything in one
>>>>>>>> go :-)
>>>>>>>>
>>>>>>>>>>>>> - it resembles hardware wires :)
>>>>>>>>>>>> This isn't for the hw wires afaiui. The hw hpd terminates in the source
>>>>>>>>>>>> bridge, which then calls drm_bridge_hpd_notify() to inform anyone else
>>>>>>>>>>>> interested in that hpd singal. This includes:
>>>>>>>>>>>> - Other bridges, e.g. if they provide CEC support.
>>>>>>>>>>>> - Other bridges, maybe they need to re-run the HDCP state engine
>>>>>>>>>>>> - Overall driver, so it can update the modes/connector status and send the
>>>>>>>>>>>> uevent to the driver.
>>>>>>>>>>>> - Overall display pipeline for this specific bridge, maybe you need to
>>>>>>>>>>>> shut down/re-enable the pipe because $reasons.
>>>>>>>>>>>>
>>>>>>>>>>>> That's at least my understanding from lots of chats with Laurent about
>>>>>>>>>>>> what he wants to do here.
>>>>>>>> That's correct, and that's what I was trying to implement :-) The
>>>>>>>> notification, in this patch series, goes from the producer bridge to a
>>>>>>>> central place (namely the connector, with a helper implementation
>>>>>>>> available as part of this series, but custom implementations in display
>>>>>>>> drivers are fine if needed) that then dispatches the notification to all
>>>>>>>> bridges (through the .lost_hotplug() operation, which we could replace
>>>>>>>> by an .hpd_notify() operation) for the first two purposes listed above,
>>>>>>>> and then to the overall driver. The only thing I don't support yet is
>>>>>>>> dispatching to the display pipeline (item 4 in the list above) as I had
>>>>>>>> no need for that, and didn't want to develop an API with no user. This
>>>>>>>> would however not be difficult to do when needed, the need is taken into
>>>>>>>> account in the proposed implementation.
>>>>>>>>
>>>>>>>>>>> I do not know the full picture, but the solution where particular bridge
>>>>>>>>>>> notifies everything unconditionally seems to me much less flexible.
>>>>>>>>>>>
>>>>>>>>>>> If HPD signals is received by the consumer, if there are no obstacles it
>>>>>>>>>>> can propagate it further, upstream bridge/encoder or to drm core - it
>>>>>>>>>>> will mimic your scenario.
>>>>>>>>>>>
>>>>>>>>>>> But there are also other scenarios where bridge does not want to
>>>>>>>>>>> propagate signal, because for example:
>>>>>>>>>>>
>>>>>>>>>>> - it wants to wait for other sinks to wake up,
>>>>>>>>>>>
>>>>>>>>>> The other sink can just do that in their hpd callback.
>>>>>>>>>>
>>>>>>>>>>> - it propagates HPD signal via hardware wire,
>>>>>>>>>> Again, the other sink can just not listen to sw hpd in that case, and use
>>>>>>>>>> the wire/hw hpd interrupt.
>>>>>>>>>>
>>>>>>>>> If it should ignore HPD, why it should receive it at all - it is
>>>>>>>>> unnecessary noise. And I am afraid with more complicated pipelines it
>>>>>>>>> will be impossible for particular component (bridge/encoder/whatever) to
>>>>>>>>> distinguish if HPD notification which came from non-directly connected
>>>>>>>>> component should be ignored or not.
>>>>>>>>>
>>>>>>>>>>> - first it wants to verify if the sink is valid/compatible/authorized
>>>>>>>>>>> device.
>>>>>>>>>> Now you lost me. Why would someone glue incompatible IP into a SoC or
>>>>>>>>>> board?
>>>>>>>>> Bridge can have external connectors, and the user can connect there
>>>>>>>>> anything.
>>>>>>>>>
>>>>>>>>>>> In general HPD is input signal for notify of state changes on particular
>>>>>>>>>>> bus, in case of typical video bridge on its output video bus.
>>>>>>>>>>>
>>>>>>>>>>> In case of bridges they have also input video buses, and they can send
>>>>>>>>>>> HPD signal via this bus, but this is indeed different HPD signal, even
>>>>>>>>>>> if for most cases they looks similar.
>>>>>>>>>> Ah, I think this is a problem we will eventually have. But it's not
>>>>>>>>>> something we're currently solving here at all I think.
>>>>>>>>> Currently sii8620 device in tm2 sends hpd signal upstream via hardware
>>>>>>>>> line, so this is not something from far future. And I guess with HPD
>>>>>>>>> broadcasting it could be racy/error prone, for example EDID reading can
>>>>>>>>> fail due to bridge being not ready (ddc of sii8620 is connected to i2c
>>>>>>>>> controller via hw wires also).
>>>>>>>>>
>>>>>>>>>>>>> And regarding implementation:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Laurent proposes to register callback drm_bridge_hpd_enable.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2. You propose to add ops hpd_notify in bridges and encoders.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Your proposition is more straightforward, but if we want to notify only
>>>>>>>>>>>>> source we should locate it by parsing notification chain (what about
>>>>>>>>>>>>> unchained bridges), or store pointer somewhere during attachment.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It still leaves us with this ugly dualism - source is encoder or bridge,
>>>>>>>>>>>>> similarly to sink as bridge or panel, but fixing it can be done later.
>>>>>>>>>>>> Uh I think we're not talking about the same thing really. My understanding
>>>>>>>>>>>> is that this callback is if someone (outside of this bridge) is interested
>>>>>>>>>>>> in a hpd signal _from_ this bridge. Which means you can only ever have 1
>>>>>>>>>>>> listener.
>>>>>>>>>>> Do we have real life examples?
>>>>>>>>>>>
>>>>>>>>>>> I want to distinguish two situations:
>>>>>>>>>>>
>>>>>>>>>>> - another device wants to know if input bus of the bridge has changed state,
>>>>>>>>>>>
>>>>>>>>>>> - another device wants to know if output bus of the bridge has changed
>>>>>>>>>>> state.
>>>>>>>>>> Uh, that's what drm_bridge_state is for (if it ever happens). That's how
>>>>>>>>>> bridges can exchange state and information about each another. hpd is
>>>>>>>>>> about the physical world, i.e. "is there a cable plugged into the port
>>>>>>>>>> I'm driving?". We're not going to use fake hpd to update bridge state and
>>>>>>>>>> fun stuff like that, we have the atomic_check machinery for this.
>>>>>>>>> My question was if we have real examples that upstream device requires
>>>>>>>>> knowledge about state of output line of the bridge?
>>>>>>>>>
>>>>>>>>> To be more precise, we have following display pipeline:
>>>>>>>>>
>>>>>>>>> A-->B-->C
>>>>>>>>>
>>>>>>>>> And C sends HPD to B (ie signal that state of line between B and C
>>>>>>>>> changed). Does A really wants to know this information? or it should
>>>>>>>>> just need to know if state of line A-->B changed?
>>>>>>>> There's one real life example, where A is an HDMI encoder, B is an HDMI
>>>>>>>> ESD protector and level shifter, and C is the physical HDMI connector.
>>>>>>>> When the HDMI cable is unplugged, the CEC controller part of A needs to
>>>>>>>> be notified in order to reset the CEC state machine. One could however
>>>>>>>> argue that in that case the A-B link state changes too, but the
>>>>>>>> important part is that HPD detection is not performed by A, while A
>>>>>>>> needs to be informed of lost hotplug.
>>>>>>> I have no full picture but I guess in this case C sends HPD to B using
>>>>>>> hardware wire, and then B sends HPD to A also via wire, so I wouldn't
>>>>>>> say that B does not participate in HPD transmission/forwarding,
>>>>>> No, in this case A doesn't receive any hardware HPD signal, it requires
>>>>>> HPD notification through software.
>>>>>>
>>>>>>> some shifters with 'advanced power saving' can even perform wake-up of
>>>>>>> upstream pin logic after receiving HPD on downstream, so HPD sent from B
>>>>>>> to A is indeed different than HPD sent from C to B.
>>>>>>>
>>>>>>> Btw, with the above logic of propagation of HPD callback (proposed by
>>>>>>> Daniel) I guess it will work this way:
>>>>>>>
>>>>>>> - A will receive HPD signal via HW,
>>>>>>>
>>>>>>> - then B and C will receive HPD callback via framework.
>>>>>>>
>>>>>>> Am I right?
>>>>>> It's the other way around.
>>>>>>
>>>>>> In this case the HPD signal from the connector (C) is routed to an input
>>>>>> of the ESD chip (B). The ESD chip outputs a shifted HPD hardware signal
>>>>>> connected to a GPIO of the SoC. The driver for (B) thus registers a GPIO
>>>>>> IRQ and receive the hardware HPD notification. The driver for the HDMI
>>>>>> encoder (A) needs to receive HPD notification in software, through the
>>>>>> framework.
>>>>> If this is GPIO I wonder why do not query this gpio by encoder directly,
>>>>> rules of ownership of such gpios seems to be grey area, so in such case
>>>>> I would advise to put it in the driver who really needs it.
>>>>>
>>>>> This way it will be much simpler.
>>>> First to fall, multiple drivers may need to be informed of HPD events
>>>> coming from a GPIO, so we would need to duplicate it in multiple places,
>>>> and I don't think the GPIO framework allows acquiring a GPIO multiple
>>>> times.
>>>>
>>>> Then, the GPIO is described in DT, and DT doesn't care about which
>>>> driver needs HPD events. DT specifies the GPIO in the node of the device
>>>> it belongs to, this is defined in DT bindings, and must be the same on
>>>> all boards, while depending on the board different devices may need to
>>>> be informed of HPD events.
>>>>
>>>> For those two reasons HPD GPIO handling and consumption of HPD events
>>>> can't always be grouped in the same driver.
>>>>
>>>>> Going back to HPD notifications, as I said earlier broadcasting HPD
>>>>> notification unconditionally to every member of the chain with hope that
>>>>> the member will be able to filter-out undesired notification seems to me
>>>>> incorrect - maybe it can solve some problems but is not flexible enough
>>>>> to be usable in other scenarios.
>>>>>
>>>>> If my arguments do not convince you please just continue with your
>>>>> ideas, we can always add NO_HPD_BROADCAST somewhere :)
>>>> :-) I would like to understand the problems you're referring to though,
>>>> and hopefully solve them. If you could describe one of the scenarios
>>>> where you think this mechanism wouldn't be usable that would help. In
>>>> the meantime I will post a new version of the series with these
>>>> operations kept as-is to get the rest of the patches reviewed.
>>> See my little thing about midlayers, I think midlayers with lots of flags
>>> for everything aren't a good idea. They should be more opinionated about
>>> how things work.
>>>
>>> So if there's a case where this broadcasting of various things doesn't
>>> work, let's dig into it.
>>> -Daniel
>> OK, almost real life example:
>>
>> A -> B -> C
>>
>> A - RGB/HDMI converter,
>>
>> B - HDMI/MHL converter,
>>
>> C - uUSB controller (MUIC).
>>
>>
>> C - detects presence of MHL sink and routes MHL lines to B in such case.
>>
>> B - has no hardware logic to detect HPD, but it's firmware can read EDID
>> from downstream component via HW lines and it has hardware lines to
>> upstream component to send EDID,
>>
>> A - can read EDID from B via hardware lines, but does not have hardware HPD.
> It probably doesn't matter much for the overall discussion, but out of
> curiosity, does B have a CBUS interface towards C and a DDC (I2C)
> interface towards A ?
Yes, but C (MUIC) is not MHL aware, beside initial 1K resistance
detection on ID pin, AFAIK.
> And does A read the EDID on DDC and expose it
> towards the SoC through a custom protocol (for instance as the ADV7511
> does), or does it forward the DDC lines to the SoC ?
>
>> So how it should work (according to specification):
>>
>> 1. C detects MHL sink.
>>
>> 2. C switches his mux to route lines to B.
>>
>> 3. C sends HPD notification to B.
>>
>> 4. B powers on, its firmware reads EDID from downstream lines (possibly
>> adjusting it) and makes it available to upstream component A.
>>
>> 5. B sends HPD notification to A.
>>
>>
>> I do not know how it could work with HPD broadcasting.
>>
>> I guess C should be HPD provider, but in case of HPD broadcasting A and
>> B would receive notification in the same time, as a result A would start
>> reading EDID too early - fail.
> That's an interesting case indeed. Now I understand what you meant
> earlier.
>
> The HPD notification from C to B is purely internal, and should not be
> visible from a DRM/KMS point of view. It just happens that this hardware
> setup has a more complex HPD sequence that requires software
> intervention in the middle of the sequence. As such, if we forget about
> this patch series for a minute, C would need a custom API to send MHL
> notification to B, and the HPD for DRM/KMS would be notified by B, right
> ?
I just want to convince you that maybe all HPD signals
(hardware/software) are purely internal (ie they should be handled only
by upstream devices), and the hotplug event should be sent to
userspace/drm_core only when WHOLE pipeline is ready to query modes
(i2c/sideband channels/whatever is functional).
>
> I think it's possible to handle both the MHL notification and the
> user-visible HPD notification through the same bridge API, provided that
> we offer a way for a bridge to block forwarding of the HPD notification.
> This will also require calling the HPD notifiers on bridges in the sink
> to source order. Both are doable, the bridge HPD notifier operation
> could return a bool that blocks propagation of the notification. Would
> that work for you ?
It could work, in this case.
But it will still have problems with non-linear pipelines - where stream
is split to two or more bridges/panels.
Regards
Andrzej
>
>>>>>>>>>>>> You seem to have some other idea here.
>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> + mutex_unlock(&bridge->hpd_mutex);
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> #ifdef CONFIG_OF
>>>>>>>>>>>>>>>>>> /**
>>>>>>>>>>>>>>>>>> * of_drm_find_bridge - find the bridge corresponding to the device node in
>>>>>>>>>>>>>>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>>>>>>>>>>>>>>>> index 08dc15f93ded..b9445aa5b1ef 100644
>>>>>>>>>>>>>>>>>> --- a/include/drm/drm_bridge.h
>>>>>>>>>>>>>>>>>> +++ b/include/drm/drm_bridge.h
>>>>>>>>>>>>>>>>>> @@ -23,8 +23,9 @@
>>>>>>>>>>>>>>>>>> #ifndef __DRM_BRIDGE_H__
>>>>>>>>>>>>>>>>>> #define __DRM_BRIDGE_H__
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -#include <linux/list.h>
>>>>>>>>>>>>>>>>>> #include <linux/ctype.h>
>>>>>>>>>>>>>>>>>> +#include <linux/list.h>
>>>>>>>>>>>>>>>>>> +#include <linux/mutex.h>
>>>>>>>>>>>>>>>>>> #include <drm/drm_mode_object.h>
>>>>>>>>>>>>>>>>>> #include <drm/drm_modes.h>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> @@ -334,6 +335,110 @@ struct drm_bridge_funcs {
>>>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>>>> void (*atomic_post_disable)(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> struct drm_atomic_state *state);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @detect:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Check if anything is attached to the bridge output.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This callback is optional, if not implemented the bridge will be
>>>>>>>>>>>>>>>>>> + * considered as always having a component attached to its output.
>>>>>>>>>>>>>>>>>> + * Bridges that implement this callback shall set the
>>>>>>>>>>>>>>>>>> + * DRM_BRIDGE_OP_DETECT flag in their &drm_bridge->ops.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * RETURNS:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * drm_connector_status indicating the bridge output status.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + enum drm_connector_status (*detect)(struct drm_bridge *bridge);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @get_modes:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Fill all modes currently valid for the sink into the &drm_connector
>>>>>>>>>>>>>>>>>> + * with drm_mode_probed_add().
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * The @get_modes callback is mostly intended to support non-probable
>>>>>>>>>>>>>>>>>> + * displays such as many fixed panels. Bridges that support reading
>>>>>>>>>>>>>>>>>> + * EDID shall leave @get_modes unimplemented and implement the
>>>>>>>>>>>>>>>>>> + * &drm_bridge_funcs->get_edid callback instead.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This callback is optional. Bridges that implement it shall set the
>>>>>>>>>>>>>>>>>> + * DRM_BRIDGE_OP_MODES flag in their &drm_bridge->ops.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * RETURNS:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * The number of modes added by calling drm_mode_probed_add().
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + int (*get_modes)(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> + struct drm_connector *connector);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @get_edid:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Read and parse the EDID data of the connected display.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * The @get_edid callback is the preferred way of reporting mode
>>>>>>>>>>>>>>>>>> + * information for a display connected to the bridge output. Bridges
>>>>>>>>>>>>>>>>>> + * that support readind EDID shall implement this callback and leave
>>>>>>>>>>>>>>>>>> + * the @get_modes callback unimplemented.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * The caller of this operation shall first verify the output
>>>>>>>>>>>>>>>>>> + * connection status and refrain from reading EDID from a disconnected
>>>>>>>>>>>>>>>>>> + * output.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This callback is optional. Bridges that implement it shall set the
>>>>>>>>>>>>>>>>>> + * DRM_BRIDGE_OP_EDID flag in their &drm_bridge->ops.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * RETURNS:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * An edid structure newly allocated with kmalloc() (or similar) on
>>>>>>>>>>>>>>>>>> + * success, or NULL otherwise. The caller is responsible for freeing
>>>>>>>>>>>>>>>>>> + * the returned edid structure with kfree().
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + struct edid *(*get_edid)(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> + struct drm_connector *connector);
>>>>>>>>>>>>>>>>> It overlaps with get_modes, I guess presence of one ops should disallow
>>>>>>>>>>>>>>>>> presence of another one?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I am not really convinced we need this op at all, cannot we just assign
>>>>>>>>>>>>>>>>> some helper function to .get_modes cb, which will do the same?
>>>>>>>>>>>>>>>> Plan B): ditch ->get_edid, require that the driver has ->get_modes in that
>>>>>>>>>>>>>>>> case, and require that if it has an edid it must fill out connector->info
>>>>>>>>>>>>>>>> and connector->edid correctly.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Btw if a hpd happens, who's responible for making sure the edid/mode list
>>>>>>>>>>>>>>>> in the connector is up-to-date? With your current callback design that's
>>>>>>>>>>>>>>>> up to the callback, which doesn't feel great. Maybe drm_bridge_hpd_notify
>>>>>>>>>>>>>>>> should guarantee that it'll first walk the connectors to update status and
>>>>>>>>>>>>>>>> edid/mode list for the final drm_connector. And then instead of just
>>>>>>>>>>>>>>>> passing the simple "status", it'll pass the connector, with everything
>>>>>>>>>>>>>>>> correctly updated.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Otherwise everyone interested in that hpd signal will go and re-fetch the
>>>>>>>>>>>>>>>> edid, which is not so awesome :-)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @lost_hotplug:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Notify the bridge of display disconnection.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This callback is optional, it may be implemented by bridges that
>>>>>>>>>>>>>>>>>> + * need to be notified of display disconnection for internal reasons.
>>>>>>>>>>>>>>>>>> + * One use case is to reset the internal state of CEC controllers for
>>>>>>>>>>>>>>>>>> + * HDMI bridges.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + void (*lost_hotplug)(struct drm_bridge *bridge);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @hpd_enable:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Enable hot plug detection. From now on the bridge shall call
>>>>>>>>>>>>>>>>>> + * drm_bridge_hpd_notify() each time a change is detected in the output
>>>>>>>>>>>>>>>>>> + * connection status, until hot plug detection gets disabled with
>>>>>>>>>>>>>>>>>> + * @hpd_disable.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This callback is optional and shall only be implemented by bridges
>>>>>>>>>>>>>>>>>> + * that support hot-plug notification without polling. Bridges that
>>>>>>>>>>>>>>>>>> + * implement it shall also implement the @hpd_disable callback and set
>>>>>>>>>>>>>>>>>> + * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + void (*hpd_enable)(struct drm_bridge *bridge);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @hpd_disable:
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * Disable hot plug detection. Once this function returns the bridge
>>>>>>>>>>>>>>>>>> + * shall not call drm_bridge_hpd_notify() when a change in the output
>>>>>>>>>>>>>>>>>> + * connection status occurs.
>>>>>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>>>>>> + * This callback is optional and shall only be implemented by bridges
>>>>>>>>>>>>>>>>>> + * that support hot-plug notification without polling. Bridges that
>>>>>>>>>>>>>>>>>> + * implement it shall also implement the @hpd_enable callback and set
>>>>>>>>>>>>>>>>>> + * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + void (*hpd_disable)(struct drm_bridge *bridge);
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> /**
>>>>>>>>>>>>>>>>>> @@ -372,6 +477,38 @@ struct drm_bridge_timings {
>>>>>>>>>>>>>>>>>> bool dual_link;
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +/**
>>>>>>>>>>>>>>>>>> + * enum drm_bridge_ops - Bitmask of operations supported by the bridge
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> +enum drm_bridge_ops {
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @DRM_BRIDGE_OP_DETECT: The bridge can detect displays connected to
>>>>>>>>>>>>>>>>>> + * its output. Bridges that set this flag shall implement the
>>>>>>>>>>>>>>>>>> + * &drm_bridge_funcs->detect callback.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + DRM_BRIDGE_OP_DETECT = BIT(0),
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @DRM_BRIDGE_OP_EDID: The bridge can retrieve the EDID of the display
>>>>>>>>>>>>>>>>>> + * connected to its output. Bridges that set this flag shall implement
>>>>>>>>>>>>>>>>>> + * the &drm_bridge_funcs->get_edid callback.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + DRM_BRIDGE_OP_EDID = BIT(1),
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @DRM_BRIDGE_OP_HPD: The bridge can detect hot-plug and hot-unplug
>>>>>>>>>>>>>>>>>> + * without requiring polling. Bridges that set this flag shall
>>>>>>>>>>>>>>>>>> + * implement the &drm_bridge_funcs->hpd_enable and
>>>>>>>>>>>>>>>>>> + * &drm_bridge_funcs->disable_hpd_cb callbacks.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + DRM_BRIDGE_OP_HPD = BIT(2),
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @DRM_BRIDGE_OP_MODES: The bridge can retrieving the modes supported
>>>>>>>>>>>>>>>>>> + * by the display at its output. This does not include readind EDID
>>>>>>>>>>>>>>>>>> + * which is separately covered by @DRM_BRIDGE_OP_EDID. Bridges that set
>>>>>>>>>>>>>>>>>> + * this flag shall implement the &drm_bridge_funcs->get_modes callback.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + DRM_BRIDGE_OP_MODES = BIT(3),
>>>>>>>>>>>>>>>>>> +};
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> /**
>>>>>>>>>>>>>>>>>> * struct drm_bridge - central DRM bridge control structure
>>>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>>>> @@ -398,6 +535,29 @@ struct drm_bridge {
>>>>>>>>>>>>>>>>>> const struct drm_bridge_funcs *funcs;
>>>>>>>>>>>>>>>>>> /** @driver_private: pointer to the bridge driver's internal context */
>>>>>>>>>>>>>>>>>> void *driver_private;
>>>>>>>>>>>>>>>>>> + /** @ops: bitmask of operations supported by the bridge */
>>>>>>>>>>>>>>>>>> + enum drm_bridge_ops ops;
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @type: Type of the connection at the bridge output
>>>>>>>>>>>>>>>>>> + * (DRM_MODE_CONNECTOR_*). For bridges at the end of this chain this
>>>>>>>>>>>>>>>>>> + * identifies the type of connected display.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + int type;
>>>>>>>>>>>>>>>>>> + /** private: */
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @hpd_mutex: Protects the @hpd_cb and @hpd_data fields.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + struct mutex hpd_mutex;
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @hpd_cb: Hot plug detection callback, registered with
>>>>>>>>>>>>>>>>>> + * drm_bridge_hpd_enable().
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + void (*hpd_cb)(void *data, enum drm_connector_status status);
>>>>>>>>>>>>>>>>>> + /**
>>>>>>>>>>>>>>>>>> + * @hpd_data: Private data passed to the Hot plug detection callback
>>>>>>>>>>>>>>>>>> + * @hpd_cb.
>>>>>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>>>>>> + void *hpd_data;
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> void drm_bridge_add(struct drm_bridge *bridge);
>>>>>>>>>>>>>>>>>> @@ -428,6 +588,14 @@ void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> void drm_atomic_bridge_enable(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> struct drm_atomic_state *state);
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +void drm_bridge_hpd_enable(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> + void (*cb)(void *data,
>>>>>>>>>>>>>>>>>> + enum drm_connector_status status),
>>>>>>>>>>>>>>>>>> + void *data);
>>>>>>>>>>>>>>>>>> +void drm_bridge_hpd_disable(struct drm_bridge *bridge);
>>>>>>>>>>>>>>>>>> +void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>>>>>>>>>>>>>>>>>> + enum drm_connector_status status);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> #ifdef CONFIG_DRM_PANEL_BRIDGE
>>>>>>>>>>>>>>>>>> struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>>>>>>>>>>>>>>>>>> u32 connector_type);
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
