Re: [PATCH 1/6] drm/dp_mst: Add drm_dp_get_payload_info()

[Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>]

On Fri, Nov 16, 2018 at 07:21:15PM -0500, Lyude Paul wrote:
> Some hardware (nvidia hardware in particular) needs to be notified of
> the exact VCPI and payload settings that the topology manager decided on
> for each mstb port. Since there isn't currently any way to get this
> information without going through port (which drivers are very much not
> supposed to do by themselves, ever), let's add one.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 56 +++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  5 ++-
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 529414556962..4336d17ce904 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1982,6 +1982,62 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr)
>  }
>  EXPORT_SYMBOL(drm_dp_update_payload_part2);
>  
> +/**
> + * drm_dp_get_payload_info() - Retrieve payload/vcpi information for the given
> + *                             @port
> + * @mgr: manager to use
> + * @port: the port to get the relevant payload information for
> + * @vcpi_out: where to copy the port's VCPI information to
> + * @payload_out: where to copy the port's payload information to
> + *
> + * Searches the current payloads for @mgr and finds the relevant payload and
> + * VCPI information that was programmed by the topology mgr, then copies it
> + * into @vcpi_out and @payload_out. Drivers which need to know this
> + * information must use this helper as opposed to checking @port themselves,
> + * as this helper will ensure the port reference is still valid and grab the
> + * appropriate locks in @mgr.
> + *
> + * Returns:
> + * 0 on success, negative error code if the port is no longer valid or a
> + * programmed payload could not be found for @port.
> + */
> +int drm_dp_get_payload_info(struct drm_dp_mst_topology_mgr *mgr,
> +			    struct drm_dp_mst_port *port,
> +			    struct drm_dp_vcpi *vcpi_out,
> +			    struct drm_dp_payload *payload_out)
> +{
> +	struct drm_dp_payload *payload = NULL;
> +	int i;
> +	int ret = 0;
> +
> +	port = drm_dp_get_validated_port_ref(mgr, port);
> +	if (!port)
> +		return -EINVAL;

This is the part that I mean in our other/irc discussions. The
dp_get_validated_port here could fail when it's going to surprise the
driver. With the dp_port_malloc_get stuff we could instead just grab a
port_malloc_kref when storing the port in mgr->payload, which would
guarantee that the port based lookup below still works.

> +
> +	mutex_lock(&mgr->payload_lock);
> +	/* Figure out which of the payloads belongs to this port */
> +	for (i = 0; i < mgr->max_payloads; i++) {
> +		if (mgr->payloads[i].vcpi == port->vcpi.vcpi) {

Or maybe even rework the lookup here to use the port pointer (as an
abstract key) instead of port->vcpi.vcpi. With port_malloc_kref we could
guarantee that it keeps working, even after the port has been destroyed.

And (without checking) I think that's needed anyway to clean up the
payload update hacks in the connector destroy work ...
-Daniel

> +			payload = &mgr->payloads[i];
> +			break;
> +		}
> +	}
> +
> +	if (!payload) {
> +		DRM_DEBUG_KMS("Failed to find payload for port %p\n", port);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	*payload_out = *payload;
> +	*vcpi_out = port->vcpi;
> +out:
> +	mutex_unlock(&mgr->payload_lock);
> +	drm_dp_put_port(port);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_dp_get_payload_info);
> +
>  #if 0 /* unused as of yet */
>  static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
>  				 struct drm_dp_mst_port *port,
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 59f005b419cf..9cc93ea60e7e 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -592,7 +592,10 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  			      struct drm_dp_mst_port *port, int pbn, int slots);
>  
>  int drm_dp_mst_get_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
> -
> +int drm_dp_get_payload_info(struct drm_dp_mst_topology_mgr *mgr,
> +			    struct drm_dp_mst_port *port,
> +			    struct drm_dp_vcpi *vcpi_out,
> +			    struct drm_dp_payload *payload_out);
>  
>  void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau