Re: [PATCH 09/60] drm/bridge: Add connector-related bridge operations and data

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Sean Paul <sean@poorly.run>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 09/60] drm/bridge: Add connector-related bridge operations and data
Date: Mon, 19 Aug 2019 05:37:36 +0300	[thread overview]
Message-ID: <20190819023736.GI4999@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20190817001456.GA20692@pendragon.ideasonboard.com>

Hi Daniel,

On Sat, Aug 17, 2019 at 03:14:56AM +0300, Laurent Pinchart wrote:
> On Sat, Aug 17, 2019 at 02:30:08AM +0300, Laurent Pinchart wrote:
> > On Wed, Aug 14, 2019 at 07:02:29PM +0200, Daniel Vetter wrote:
> >> On Wed, Aug 14, 2019 at 04:30:57PM +0300, Laurent Pinchart wrote:
> >>> On Wed, Aug 14, 2019 at 03:03:29PM +0200, Daniel Vetter wrote:
> >>>> On Thu, Aug 08, 2019 at 09:36:31PM +0300, Laurent Pinchart wrote:
> >>>>> On Thu, Aug 08, 2019 at 09:19:48PM +0300, Laurent Pinchart wrote:
> >>>>>> On Thu, Jul 11, 2019 at 09:35:48AM +0200, 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.
> >>>>>> 
> >>>>>> The reason is that a bridge may support an operation (as in implemented
> >>>>>> in the bridge hardware), but that operation may not be supported on a
> >>>>>> particular board. For instance an HDMI encoder may support reading EDID
> >>>>>> when the DDC lines are connected to the encoder, but a board may connect
> >>>>>> the DDC lines to an I2C port of the SoC. We thus need to decouple
> >>>>>> if a particular instance of the device supports the operation (exposed
> >>>>>> by the ops flags) from the function pointers.
> >>>>>> 
> >>>>>> We could of course allocate the drm_bridge_funcs structure dynamically
> >>>>>> for each bridge instance, and fill it with function pointers manually,
> >>>>>> leaving the unused ops always NULL, but that would require making the
> >>>>>> structure writable, which is considered a security issue. That's why I
> >>>>>> decided to keep the drm_bridge_funcs structure as a global static const
> >>>>>> structure, and add an ops bitmask.
> >>>>>> 
> >>>>>>>>> - 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.
> >>>>>> 
> >>>>>> No, the whole point is that they should not be chained at all. A bridge
> >>>>>> does not have to propagate, for instance, .get_edid() to the next
> >>>>>> bridge. That's one of the core design principles in this series, I want
> >>>>>> to keep the bridges as simple as possible, and move the complexity of
> >>>>>> the boilerplate code that is currently copied all around to helpers. See
> >>>>>> patch "drm: Add helper to create a connector for a chain of bridges" for
> >>>>>> more information about how this is used, with a helper that delegates
> >>>>>> the connector operations to the correct bridge in the chain based on the
> >>>>>> ops reported by each bridge.
> >>>>>> 
> >>>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>>>>> ---
> >>>>>>>>>  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?
> >>>>>> 
> >>>>>> That's why I decided to hide hide HPD through helpers,
> >>>>>> drm_bridge_hpd_enable() and drm_bridge_hpd_disable() on the listener
> >>>>>> side, and drm_bridge_hpd_notify() on the event reporter side. While the
> >>>>>> current implementation is limited to a single listener, only the helpers
> >>>>>> would need to be changed to extend that to multiple listeners.
> >>>>>> 
> >>>>>> Note that the .hpd_enable() and .hpd_disable() operations also allow the
> >>>>>> bridge to disable HPD detection when not used. Doing so keeps the bridge
> >>>>>> simple, it only needs to care about reporting HPD events when they're
> >>>>>> enabled, without caring who (if anyone) is listening, and gets clear
> >>>>>> instructions on whether to enable or disable the HPD hardware (in case
> >>>>>> it can be disabled).
> >>>>>> 
> >>>>>>>> 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?
> >>>>>> 
> >>>>>> This is something this series doesn't implement. I don't think it would
> >>>>>> be a big deal, but my knowledge of HPD (especially for DisplayPort) ends
> >>>>>> here. If you can elaborate on what would be needed, I can implement
> >>>>>> that.
> >>>>>> 
> >>>>>>>>> +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.
> >>>>>> 
> >>>>>> I'll reply to this further down the mail thread, to address additional
> >>>>>> comments.
> >>>>>> 
> >>>>>>>>> +	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.
> >>>>>> 
> >>>>>> I think that's doable, I'll have a look.
> >>>>> 
> >>>>> So I had a look, and while this is doable, it would essentially mean
> >>>>> that all bridges that retrieve modes from EDID would have to roll out
> >>>>> their own version of the following code:
> >>>>> 
> >>>>> static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
> >>>>> 					       struct drm_bridge *bridge)
> >>>>> {
> >>>>> 	enum drm_connector_status status;
> >>>>> 	struct edid *edid;
> >>>>> 	int n;
> >>>>> 
> >>>>> 	status = drm_bridge_connector_detect(connector, false);
> >>>>> 	if (status != connector_status_connected)
> >>>>> 		goto no_edid;
> >>>>> 
> >>>>> 	edid = bridge->funcs->get_edid(bridge, connector);
> >>>>> 	if (!edid || !drm_edid_is_valid(edid)) {
> >>>>> 		kfree(edid);
> >>>>> 		goto no_edid;
> >>>>> 	}
> >>>>> 
> >>>>> 	drm_connector_update_edid_property(connector, edid);
> >>>>> 	n = drm_add_edid_modes(connector, edid);
> >>>>> 
> >>>>> 	kfree(edid);
> >>>>> 	return n;
> >>>>> 
> >>>>> no_edid:
> >>>>> 	drm_connector_update_edid_property(connector, NULL);
> >>>>> 	return 0;
> >>>>> }
> >>>>> 
> >>>>> Is this desired ?
> >>>> 
> >>>> We store the edid, and we store a lot of decoded information in
> >>>> drm_connector->display_info. Can't they just look there? Re-fetching the
> >>>> edid definitely sounds like the wrong thing to do.
> >>>> 
> >>>> We might run into some ordering issue here I guess with hotplugs and who's
> >>>> fetching the edid and everything like that.
> >>> 
> >>> That's exactly what I was about to answer after reading your first
> >>> paragraph :-) I believe caching EDID is a good idea, but my familiarity
> >>> with hotplug-related issues is limited to a handful of systems, and I'm
> >>> sure I'm missing some common problems. If you can tell me how you think
> >>> this should be done, I can give it a try.
> >> 
> >> I think all you need to do is make sure that when handling a hpd, the edid
> >> is fetched first. Before other parts of the bridge try to reconfigure
> >> themselves ...
> >> 
> >>>> Also maybe I'm missing the point here, and thinking too much of
> >>>> ->get_modes on the connector. But then I'm not clear on why the bridge
> >>>> needs the connector, and why it instead can't just return the edid it can
> >>>> read and let the caller/core figure out everything else?
> >>> 
> >>> That's exactly what the .get_edid() operation that you asked me to
> >>> remove did... :-) You didn't like the fact that it duplicated the
> >>> .get_modes() logic. Should I add it back, and clearly document
> >>> .get_modes() as a fallback used only when the connector doesn't use EDID
> >>> ?
> >> 
> >> I guess I'm making a bit a fool of myself here. What I meant is that if we
> >> do want to keep ->get_edid, then why do you need to pass the connector?
> >> Just return the edid blob, and let the caller parse it, and stuff all
> >> relevant data into drm_connector. Just thinking along the lines of your
> >> goal of making the bridge drivers as dumb as possible.
> >> 
> >> Same thing for ->get_modes would be neat too, but we don't have a nice
> >> datastructure for this. We'd need to pass both a list_head and a pointer
> >> to the drm_display_info I think.
> >> 
> >> That would decouple bridges even more from connector, which I think is
> >> somewhere on your goal list ...
> > 
> > So I had a look at that. We would need to remove the connector argument
> > from drm_do_get_edid(). The connector currently stores a few fields
> > related to EDID parsing:
> > 
> >         /**
> >          * @null_edid_counter: track sinks that give us all zeros for the EDID.
> >          * Needed to workaround some HW bugs where we get all 0s
> >          */
> >         int null_edid_counter;
> > 
> >         /** @bad_edid_counter: track sinks that give us an EDID with invalid checksum */
> >         unsigned bad_edid_counter;
> > 
> >         /**
> >          * @edid_corrupt: Indicates whether the last read EDID was corrupt. Used
> >          * in Displayport compliance testing - Displayport Link CTS Core 1.2
> >          * rev1.1 4.2.2.6
> >          */
> >         bool edid_corrupt;
> > 
> > We would need to decouple that from drm_connector. One option would be
> > to create a new drm_edid structure that would store those three fields,
> > as well as a struct edid, and return it from drm_do_get_edid() instead
> > of the raw struct edid. Would you prefer a different solution ? Do you
> > think that's a prerequisite to for this patch ?
> 
> The more I look at EDID parsing, the more it feels that we should
> redesign the whole HPD and EDID get handling, and the more it becomes
> out of scope for this patch series :-S EDID retrieval and extraction of
> information from EDID is intertwined, for instance drm_get_edid()
> performs the following:
> 
> struct edid *drm_get_edid(struct drm_connector *connector,
> 			  struct i2c_adapter *adapter)
> {
> 	struct edid *edid;
> 
> 	if (connector->force == DRM_FORCE_OFF)
> 		return NULL;
> 
> 	if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter))
> 		return NULL;
> 
> 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> 	if (edid)
> 		drm_get_displayid(connector, edid);
> 	return edid;
> }
> 
> I don't think the drm_get_displayid() call belongs there. Moving it to
> the numerous callers of drm_get_edid() doesn't seem a good idea. Ideally
> it should be done at the same time as populating the modes from EDID,
> but I'm pretty sure that would break things, with not all EDID retrieval
> resulting in modes updates.
> 
> A few drivers call drm_do_get_edid() directly, in order to provide a
> custom EDID block read function, and they skip connector->force handling
> and drm_get_displayid() as a result. In most case I assume that's a bug
> that just went unnoticed.
> 
> Decoupling EDID read from drm_connector for bridges in a proper way seem
> like a huge piece of work to me, and I really can't make it a
> prerequisite for this patch.

I still gave it a try, and it resulted in

	git://linuxtv.org/pinchartl/media.git omapdrm/edid

Could you have a look at the last five patches in the branch ?

drm/edid: Reorganise the DisplayID parsing code
drm/edid: Move functions to avoid forward declaration
drm/edid: Move DisplayID tile parsing to drm_connector.c
drm/edid: Honour connector->force in drm_do_get_edid()
[WIP] drm/edid: Decouple EDID retrieval from connector

While the first four patches probably make sense and could be merged
independently of the last one, it's really the fifth patch that makes
decoupling of .get_edid() from drm_connector possible. And it's the
patch I'm the least happy with in this whole series :-S As written in
it's commit message, is it worth it ?

> >>>>>>> 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 :-)
> >>>>>> 
> >>>>>> With the current design there's a single listener, so it's not a big
> >>>>>> deal :-) Furthermore, the listener is the helper that creates a
> >>>>>> connector on top of a chain of bridges, so it's a pretty good place to
> >>>>>> handle this. See the call to drm_kms_helper_hotplug_event() in
> >>>>>> drm_bridge_connector_hpd_cb().
> >>>>>> 
> >>>>>> I'm all for reworking HPD and mode fetching, but I think it's a bit too
> >>>>>> big of a requirement as a prerequisite for this series (or as part of
> >>>>>> this series). We have hardware that can report HPD with various level of
> >>>>>> details (from "something happened on a connector" to "this particular
> >>>>>> event happened on this particular connector"), and we channel that
> >>>>>> through helpers such as drm_kms_helper_hotplug_event() that lose the
> >>>>>> details and go through a heavy mechanism to refetch everything. I
> >>>>>> understand this is needed in many cases, but I think there's room for
> >>>>>> improvement. This series, in my opinion, doesn't go in the wrong
> >>>>>> direction in that regard, as it eventually calls
> >>>>>> drm_kms_helper_hotplug_event(), so I think improvements would make sense
> >>>>>> on top of it. I'm even willing to work on this, provided I get feedback
> >>>>>> on what is desired.
> >>>>>> 
> >>>>>>>>> +	/**
> >>>>>>>>> +	 * @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);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel