Re: [PATCH] drm/dp_mst: Remove drm_dp_mst_has_audio()

From: Sean Paul
Date: Mon Apr 06 2020 - 15:56:57 EST

On Fri, Apr 3, 2020 at 6:23 PM Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> Drive-by fix I noticed the other day - drm_dp_mst_has_audio() only ever
> made sense back when we still had to validate ports before accessing
> them in order to (attempt to) avoid NULL dereferences. Since we have
> proper reference counting that guarantees we always can safely access
> the MST port, there's no use in keeping this function around as all it
> does is validate the port pointer before checking the audio status.
>
> Note - drm_dp_mst_port->has_audio is technically protected by
> drm_device->mode_config.connection_mutex, since it's only ever updated
> from drm_dp_mst_get_edid(). Additionally, we change the declaration for
> port in struct intel_connector to be properly typed, so we can directly
> access it.
>

This is kind of burying the lede. I'd almost prefer a 2 patch series:

drm/i915: Allow connectors to directly access drm_dp_mst_port
drm/dp_mst: Remove unused drm_dp_mst_port_has_audio()

That way if anyone objects to the idea of accessing mst_port directly
from i915 driver, it's more obvious from the patch subject.

Nitpicks aside, the code looks good to me, it's a nice cleanup!

Reviewed-by: Sean Paul <sean@xxxxxxxxxx>

> Cc: "Lee, Shawn C" <shawn.c.lee@xxxxxxxxx>
> Cc: Sean Paul <sean@xxxxxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 21 -------------------
> .../drm/i915/display/intel_display_debugfs.c | 10 ++-------
> .../drm/i915/display/intel_display_types.h | 2 +-
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +--
> include/drm/drm_dp_mst_helper.h | 2 --
> 5 files changed, 4 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 1ff49547b2e8..129126091e90 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4063,27 +4063,6 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_dp_mst_detect_port);
>
> -/**
> - * drm_dp_mst_port_has_audio() - Check whether port has audio capability or not
> - * @mgr: manager for this port
> - * @port: unverified pointer to a port.
> - *
> - * This returns whether the port supports audio or not.
> - */
> -bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,
> - struct drm_dp_mst_port *port)
> -{
> - bool ret = false;
> -
> - port = drm_dp_mst_topology_get_port_validated(mgr, port);
> - if (!port)
> - return ret;
> - ret = port->has_audio;
> - drm_dp_mst_topology_put_port(port);
> - return ret;
> -}
> -EXPORT_SYMBOL(drm_dp_mst_port_has_audio);
> -
> /**
> * drm_dp_mst_get_edid() - get EDID for an MST port
> * @connector: toplevel connector to get EDID for
> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> index 424f4e52f783..9f736420d83f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> @@ -631,15 +631,9 @@ static void intel_dp_info(struct seq_file *m,
> }
>
> static void intel_dp_mst_info(struct seq_file *m,
> - struct intel_connector *intel_connector)
> + struct intel_connector *intel_connector)
> {
> - struct intel_encoder *intel_encoder = intel_attached_encoder(intel_connector);
> - struct intel_dp_mst_encoder *intel_mst =
> - enc_to_mst(intel_encoder);
> - struct intel_digital_port *intel_dig_port = intel_mst->primary;
> - struct intel_dp *intel_dp = &intel_dig_port->dp;
> - bool has_audio = drm_dp_mst_port_has_audio(&intel_dp->mst_mgr,
> - intel_connector->port);
> + bool has_audio = intel_connector->port->has_audio;
>
> seq_printf(m, "\taudio support: %s\n", yesno(has_audio));
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 2bedd626c686..1de7bef0a49b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -436,7 +436,7 @@ struct intel_connector {
> state of connector->polled in case hotplug storm detection changes it */
> u8 polled;
>
> - void *port; /* store this opaque as its illegal to dereference it */
> + struct drm_dp_mst_port *port;
>
> struct intel_dp *mst_port;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 61605eb8c2af..c35efc9e628d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -114,8 +114,7 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>
> if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
> pipe_config->has_audio =
> - drm_dp_mst_port_has_audio(&intel_dp->mst_mgr,
> - connector->port);
> + connector->port->has_audio;
> else
> pipe_config->has_audio =
> intel_conn_state->force_audio == HDMI_AUDIO_ON;
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 7af51c947b81..2d7c26592c05 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -732,8 +732,6 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
> struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port);
>
> -bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,
> - struct drm_dp_mst_port *port);
> struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
>
>
> --
> 2.25.1
>