Message-ID: <d4541684-337f-4c3f-fafa-a883be370c0e@linaro.org>
Date:   Wed, 4 May 2022 13:41:49 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Douglas Anderson <dianders@...omium.org>,
        dri-devel@...ts.freedesktop.org
Cc:     Philip Chen <philipchen@...omium.org>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        Stephen Boyd <swboyd@...omium.org>,
        freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Robert Foss <robert.foss@...aro.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Alex Deucher <alexander.deucher@....com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>, Lyude Paul <lyude@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] drm/dp: Add callbacks to make using DP AUX bus
 properly easier

On 04/05/2022 01:40, Douglas Anderson wrote:
> As talked about in this patch in the kerneldoc of
> of_dp_aux_populate_ep_device() and also in the past in commit
> a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to
> its own sub-dev"), it can be difficult for eDP controller drivers to
> know when the panel has finished probing when they're using
> of_dp_aux_populate_ep_devices().
> 
> The ti-sn65dsi86 driver managed to solve this because it was already
> broken up into a bunch of sub-drivers. That means we could solve the
> problem there by adding a new sub-driver to get the panel. We could
> use the traditional -EPROBE_DEFER retry mechansim to handle the case
> where the panel hadn't probed yet.
> 
> In parade-ps8640 we didn't really solve this. The code just expects
> the panel to be ready right away. While reviewing the code originally
> I had managed to convince myself it was fine to just expect the panel
> right away, but additional testing has shown that not to be the
> case. We could fix parade-ps8640 like we did ti-sn65dsi86 but it's
> pretty cumbersome (since we're not already broken into multiple
> drivers) and requires a bunch of boilerplate code.
> 
> After discussion [1] it seems like the best solution for most people
> is:
> - Accept that there's always at most one device that will probe as a
>    result of the DP AUX bus (it may have sub-devices, but there will be
>    one device _directly_ probed).
> - When that device finishes probing, we can just have a call back.
> 
> This patch implements that idea. We'll now take a callback as an
> argument to the populate function. To make this easier to land in
> pieces, we'll make wrappers for the old functions. The functions with
> the new name (which make it clear that we only have one child) will
> take the callback and the functions with the old name will temporarily
> wrap.
> 
> This patch also includes in a few tiny fixes that I noticed while
> working on the code. Specifically:
> - We had forgotten a EXPORT_SYMBOL_GPL on the non "devm" populate
>    function.

This can go to a separate patch, so that the fix can be backported to 
earlier kernels. Please don't forget the Fixes: tag.

> - The kerneldoc of dp_aux_ep_dev_release() had incorrectly included a
>    "Returns" clause.
> 
> [1] https://lore.kernel.org/r/CAD=FV=Ur3afHhsXe7a3baWEnD=MFKFeKRbhFU+bt3P67G0MVzQ@mail.gmail.com
> 
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> 
> Changes in v2:
> - Change to assume exactly one device.
> - Have a probe callback instead of an extra sub device.
> 
>   drivers/gpu/drm/display/drm_dp_aux_bus.c | 215 +++++++++++++++--------
>   include/drm/display/drm_dp_aux_bus.h     |  24 ++-
>   2 files changed, 164 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> index dccf3e2ea323..f3412c1c2a3c 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> @@ -3,10 +3,10 @@
>    * Copyright 2021 Google Inc.
>    *
>    * The DP AUX bus is used for devices that are connected over a DisplayPort
> - * AUX bus. The devices on the far side of the bus are referred to as
> - * endpoints in this code.
> + * AUX bus. The device on the far side of the bus is referred to as an
> + * endpoint in this code.
>    *
> - * Commonly there is only one device connected to the DP AUX bus: a panel.
> + * There is only one device connected to the DP AUX bus: an eDP panel.
>    * Though historically panels (even DP panels) have been modeled as simple
>    * platform devices, putting them under the DP AUX bus allows the panel driver
>    * to perform transactions on that bus.
> @@ -22,6 +22,11 @@
>   #include <drm/display/drm_dp_aux_bus.h>
>   #include <drm/display/drm_dp_helper.h>
>   
> +struct dp_aux_ep_device_with_data {
> +	struct dp_aux_ep_device aux_ep;
> +	int (*done_probing)(struct drm_dp_aux *aux);
> +};
> +
>   /**
>    * dp_aux_ep_match() - The match function for the dp_aux_bus.
>    * @dev: The device to match.
> @@ -48,6 +53,8 @@ static int dp_aux_ep_probe(struct device *dev)
>   {
>   	struct dp_aux_ep_driver *aux_ep_drv = to_dp_aux_ep_drv(dev->driver);
>   	struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
> +	struct dp_aux_ep_device_with_data *aux_ep_with_data =
> +		container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep);
>   	int ret;
>   
>   	ret = dev_pm_domain_attach(dev, true);
> @@ -56,7 +63,31 @@ static int dp_aux_ep_probe(struct device *dev)
>   
>   	ret = aux_ep_drv->probe(aux_ep);
>   	if (ret)
> -		dev_pm_domain_detach(dev, true);
> +		goto err_attached;
> +
> +	if (aux_ep_with_data->done_probing) {
> +		ret = aux_ep_with_data->done_probing(aux_ep->aux);
> +		if (ret) {
> +			/*
> +			 * The done_probing() callback can only defer if it's
> +			 * called directly from of_dp_aux_populate_ep_device()
> +			 * because we had no DP AUX children. Flag an error.
> +			 */
> +			if (ret == -EPROBE_DEFER) {
> +				dev_err(dev,
> +					"DP AUX done_probing() can't defer w/ aux children\n");
> +				ret = -EINVAL;
> +			}
> +			goto err_probed;
> +		}
> +	}
> +
> +	return 0;
> +err_probed:
> +	if (aux_ep_drv->remove)
> +		aux_ep_drv->remove(aux_ep);
> +err_attached:
> +	dev_pm_domain_detach(dev, true);
>   
>   	return ret;
>   }
> @@ -66,7 +97,6 @@ static int dp_aux_ep_probe(struct device *dev)
>    * @dev: The device to remove.
>    *
>    * Calls through to the endpoint driver remove.
> - *
>    */
>   static void dp_aux_ep_remove(struct device *dev)
>   {
> @@ -120,12 +150,14 @@ ATTRIBUTE_GROUPS(dp_aux_ep_dev);
>   /**
>    * dp_aux_ep_dev_release() - Free memory for the dp_aux_ep device
>    * @dev: The device to free.
> - *
> - * Return: 0 if no error or negative error code.
>    */
>   static void dp_aux_ep_dev_release(struct device *dev)
>   {
> -	kfree(to_dp_aux_ep_dev(dev));
> +	struct dp_aux_ep_device *aux_ep = to_dp_aux_ep_dev(dev);
> +	struct dp_aux_ep_device_with_data *aux_ep_with_data =
> +		container_of(aux_ep, struct dp_aux_ep_device_with_data, aux_ep);
> +
> +	kfree(aux_ep_with_data);
>   }
>   
>   static struct device_type dp_aux_device_type_type = {
> @@ -139,12 +171,14 @@ static struct device_type dp_aux_device_type_type = {
>    * @dev: The device to destroy.
>    * @data: Not used
>    *
> - * This is just used as a callback by of_dp_aux_depopulate_ep_devices() and
> + * This is just used as a callback by of_dp_aux_depopulate_ep_device() and
>    * is called for _all_ of the child devices of the device providing the AUX bus.
>    * We'll only act on those that are of type "dp_aux_bus_type".
>    *
> - * This function is effectively an inverse of what's in the loop
> - * in of_dp_aux_populate_ep_devices().
> + * This function is effectively an inverse of what's in
> + * of_dp_aux_populate_ep_devices(). NOTE: since we only populate one child
> + * then it's expected that only one device will match all the "if" tests in
> + * this function and get to the device_unregister().
>    *
>    * Return: 0 if no error or negative error code.
>    */
> @@ -167,122 +201,159 @@ static int of_dp_aux_ep_destroy(struct device *dev, void *data)
>   }
>   
>   /**
> - * of_dp_aux_depopulate_ep_devices() - Undo of_dp_aux_populate_ep_devices
> - * @aux: The AUX channel whose devices we want to depopulate
> + * of_dp_aux_depopulate_ep_device() - Undo of_dp_aux_populate_ep_device
> + * @aux: The AUX channel whose device we want to depopulate
>    *
> - * This will destroy all devices that were created
> - * by of_dp_aux_populate_ep_devices().
> + * This will destroy the device that was created
> + * by of_dp_aux_populate_ep_device().
>    */
> -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux)
> +void of_dp_aux_depopulate_ep_device(struct drm_dp_aux *aux)
>   {
>   	device_for_each_child_reverse(aux->dev, NULL, of_dp_aux_ep_destroy);
>   }
> -EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_devices);
> +EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_device);

Small note about the name. What if we change that to something more 
future-proof? Something like of_dp_aux_depopulate_bus() (and similarly 
rename other calls)?

>   
>   /**
> - * of_dp_aux_populate_ep_devices() - Populate the endpoint devices on the DP AUX
> - * @aux: The AUX channel whose devices we want to populate. It is required that
> + * of_dp_aux_populate_ep_device() - Populate the endpoint device on the DP AUX
> + * @aux: The AUX channel whose device we want to populate. It is required that
>    *       drm_dp_aux_init() has already been called for this AUX channel.
> + * @done_probing: Callback functions to call after EP device finishes probing.
> + *                If we have no EP devices this will be called directly.
> + *                NULL if you don't need a callback.
>    *
> - * This will populate all the devices under the "aux-bus" node of the device
> - * providing the AUX channel (AKA aux->dev).
> + * This will populate the device (expected to be an eDP panel) under the
> + * "aux-bus" node of the device providing the AUX channel (AKA aux->dev).
>    *
>    * When this function finishes, it is _possible_ (but not guaranteed) that
> - * our sub-devices will have finished probing. It should be noted that if our
> - * sub-devices return -EPROBE_DEFER that we will not return any error codes
> - * ourselves but our sub-devices will _not_ have actually probed successfully
> - * yet. There may be other cases (maybe added in the future?) where sub-devices
> - * won't have been probed yet when this function returns, so it's best not to
> - * rely on that.
> + * our sub-device will have finished probing. It should be noted that if our
> + * sub-device returns -EPROBE_DEFER or is probing asynchronously for some
> + * reason that we will not return any error codes ourselves but our
> + * sub-device will _not_ have actually probed successfully yet.
> + *
> + * In many cases it's important for the caller of this function to be notified
> + * when our sub device finishes probing. Our sub device is expected to be an
> + * eDP panel and the caller is expected to be an eDP controller. The eDP
> + * controller needs to be able to get a reference to the panel when it finishes
> + * probing. For this reason the caller can pass in a function pointer that
> + * will be called when our sub-device finishes probing.
>    *
>    * If this function succeeds you should later make sure you call
> - * of_dp_aux_depopulate_ep_devices() to undo it, or just use the devm version
> + * of_dp_aux_depopulate_ep_device() to undo it, or just use the devm version
>    * of this function.
>    *
>    * Return: 0 if no error or negative error code.
>    */
> -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +int of_dp_aux_populate_ep_device(struct drm_dp_aux *aux,
> +				 int (*done_probing)(struct drm_dp_aux *aux))
>   {
> -	struct device_node *bus, *np;
> +	struct device_node *bus = NULL, *np = NULL;
>   	struct dp_aux_ep_device *aux_ep;
> +	struct dp_aux_ep_device_with_data *aux_ep_with_data;
>   	int ret;
>   
>   	/* drm_dp_aux_init() should have been called already; warn if not */
>   	WARN_ON_ONCE(!aux->ddc.algo);
>   
> -	if (!aux->dev->of_node)
> -		return 0;
> +	if (aux->dev->of_node)
> +		bus = of_get_child_by_name(aux->dev->of_node, "aux-bus");
> +	if (bus) {
> +		np = of_get_next_available_child(bus, NULL);
> +		of_node_put(bus);
> +	}
>   
> -	bus = of_get_child_by_name(aux->dev->of_node, "aux-bus");
> -	if (!bus)
> +	/*
> +	 * If no parent "of_node", no "aux-bus" child node, or no available
> +	 * children then we're done. Call the callback (if needed) and return.
> +	 *
> +	 * NOTE: we intentionally pass the return code from done_probing
> +	 * directly out here. eDP controller drivers may want to support
> +	 * panels from old device trees where the panel was an independent
> +	 * platform device. In that case it's expected that done_probing()
> +	 * might need to return -EPROBE_DEFER to our caller.
> +	 */
> +	if (!np) {
> +		if (done_probing)
> +			return done_probing(aux);

I see your point here (and that it makes code simpler). However I'm a 
little bit uneasy here. What if code this more explicitly in the 
drivers? Like the following:

if (!dev_has_aux_bus()) {
	ret = panel_ready(....);
} else {
	...
	ret = of_dp_aux_populate_ep_device(dp_aux, panel_ready);
	....;
}

This way you won't have to worry about the EPROBE_DEFER. Or you'd rather 
forbid it explicitly. Why? Consider the following scenario:

dp_driver_probe()
   /* This creates new devices */
   done_probing returns -EPROBE_DEFER
   /* device registration is unwound */
   dp_driver_probe returns -EPROBE_DEFER

However as the state of devices was chagned, the dp_driver_probe() can 
be called again and again, ending up with the the same probe loop that 
we are trying to solve.


>   		return 0;
> +	}
>   
> -	for_each_available_child_of_node(bus, np) {
> -		if (of_node_test_and_set_flag(np, OF_POPULATED))
> -			continue;
> +	if (of_node_test_and_set_flag(np, OF_POPULATED)) {
> +		dev_err(aux->dev, "DP AUX EP device already populated\n");
> +		ret = -EINVAL;
> +		goto err_did_get_np;
> +	}
>   
> -		aux_ep = kzalloc(sizeof(*aux_ep), GFP_KERNEL);
> -		if (!aux_ep)
> -			continue;
> -		aux_ep->aux = aux;
> +	aux_ep_with_data = kzalloc(sizeof(*aux_ep_with_data), GFP_KERNEL);
> +	if (!aux_ep_with_data) {
> +		ret = -ENOMEM;
> +		goto err_did_set_populated;
> +	}
>   
> -		aux_ep->dev.parent = aux->dev;
> -		aux_ep->dev.bus = &dp_aux_bus_type;
> -		aux_ep->dev.type = &dp_aux_device_type_type;
> -		aux_ep->dev.of_node = of_node_get(np);
> -		dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev));
> +	aux_ep_with_data->done_probing = done_probing;
>   
> -		ret = device_register(&aux_ep->dev);
> -		if (ret) {
> -			dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret);
> -			of_node_clear_flag(np, OF_POPULATED);
> -			of_node_put(np);
> +	aux_ep = &aux_ep_with_data->aux_ep;
> +	aux_ep->aux = aux;
> +	aux_ep->dev.parent = aux->dev;
> +	aux_ep->dev.bus = &dp_aux_bus_type;
> +	aux_ep->dev.type = &dp_aux_device_type_type;
> +	aux_ep->dev.of_node = of_node_get(np);
> +	dev_set_name(&aux_ep->dev, "aux-%s", dev_name(aux->dev));
>   
> -			/*
> -			 * As per docs of device_register(), call this instead
> -			 * of kfree() directly for error cases.
> -			 */
> -			put_device(&aux_ep->dev);
> +	ret = device_register(&aux_ep->dev);
> +	if (ret) {
> +		dev_err(aux->dev, "Failed to create AUX EP for %pOF: %d\n", np, ret);
>   
> -			/*
> -			 * Following in the footsteps of of_i2c_register_devices(),
> -			 * we won't fail the whole function here--we'll just
> -			 * continue registering any other devices we find.
> -			 */
> -		}
> -	}
> +		/*
> +		 * As per docs of device_register(), call this instead
> +		 * of kfree() directly for error cases.
> +		 */
> +		put_device(&aux_ep->dev);
>   
> -	of_node_put(bus);
> +		goto err_did_set_populated;
> +	}
>   
>   	return 0;
> +
> +err_did_set_populated:
> +	of_node_clear_flag(np, OF_POPULATED);
> +
> +err_did_get_np:
> +	of_node_put(np);
> +
> +	return ret;
>   }
> +EXPORT_SYMBOL_GPL(of_dp_aux_populate_ep_device);
>   
> -static void of_dp_aux_depopulate_ep_devices_void(void *data)
> +static void of_dp_aux_depopulate_ep_device_void(void *data)
>   {
> -	of_dp_aux_depopulate_ep_devices(data);
> +	of_dp_aux_depopulate_ep_device(data);
>   }
>   
>   /**
> - * devm_of_dp_aux_populate_ep_devices() - devm wrapper for of_dp_aux_populate_ep_devices()
> - * @aux: The AUX channel whose devices we want to populate
> + * devm_of_dp_aux_populate_ep_device() - devm wrapper for of_dp_aux_populate_ep_device()
> + * @aux: The AUX channel whose device we want to populate
> + * @done_probing: Callback functions to call after EP device finishes probing.
> + *                If we have no EP devices this will be called directly.
> + *                NULL if you don't need a callback.
>    *
>    * Handles freeing w/ devm on the device "aux->dev".
>    *
>    * Return: 0 if no error or negative error code.
>    */
> -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +int devm_of_dp_aux_populate_ep_device(struct drm_dp_aux *aux,
> +				      int (*done_probing)(struct drm_dp_aux *aux))
>   {
>   	int ret;
>   
> -	ret = of_dp_aux_populate_ep_devices(aux);
> +	ret = of_dp_aux_populate_ep_device(aux, done_probing);
>   	if (ret)
>   		return ret;
>   
>   	return devm_add_action_or_reset(aux->dev,
> -					of_dp_aux_depopulate_ep_devices_void,
> +					of_dp_aux_depopulate_ep_device_void,
>   					aux);
>   }
> -EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_devices);
> +EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_ep_device);
>   
>   int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *drv, struct module *owner)
>   {
> diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
> index 4f19b20b1dd6..c6d5a66f75f2 100644
> --- a/include/drm/display/drm_dp_aux_bus.h
> +++ b/include/drm/display/drm_dp_aux_bus.h
> @@ -44,9 +44,27 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
>   	return container_of(drv, struct dp_aux_ep_driver, driver);
>   }
>   
> -int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux);
> -void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux);
> -int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux);
> +int of_dp_aux_populate_ep_device(struct drm_dp_aux *aux,
> +				 int (*done_probing)(struct drm_dp_aux *aux));
> +void of_dp_aux_depopulate_ep_device(struct drm_dp_aux *aux);
> +int devm_of_dp_aux_populate_ep_device(struct drm_dp_aux *aux,
> +				      int (*done_probing)(struct drm_dp_aux *aux));
> +
> +/* Deprecated versions of the above functions. To be removed when no callers. */
> +static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +{
> +	return of_dp_aux_populate_ep_device(aux, NULL);
> +}
> +
> +static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +{
> +	return devm_of_dp_aux_populate_ep_device(aux, NULL);
> +}
> +
> +static inline void of_dp_aux_depopulate_ep_devices(struct drm_dp_aux *aux)
> +{
> +	of_dp_aux_depopulate_ep_device(aux);
> +}
>   
>   #define dp_aux_dp_driver_register(aux_ep_drv) \
>   	__dp_aux_dp_driver_register(aux_ep_drv, THIS_MODULE)


-- 
With best wishes
Dmitry

Powered by blists - more mailing lists