From: Jani Nikula <jani.nikula@intel.com>
To: Imre Deak <imre.deak@intel.com>, intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 11/11] drm/i915/dp_mst: Use intel_connector vs. drm_connector pointer in intel_dp_mst.c
Date: Thu, 12 Dec 2024 12:25:38 +0200 [thread overview]
Message-ID: <87ttb9w63h.fsf@intel.com> (raw)
In-Reply-To: <20241211230328.4012496-12-imre.deak@intel.com>
On Thu, 12 Dec 2024, Imre Deak <imre.deak@intel.com> wrote:
> Follow the canonical way in intel_dp_mst.c, referencing a connector only
> via a struct intel_connector pointer and naming this pointer 'connector'
> instead of 'intel_connector', the only exception being the casting of
> a drm_connector function parameter pointer to intel_connector, calling
> the drm_connector pointer _connector.
>
> Suggested-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
A few issues below that should be addressed, with those fixed,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 180 ++++++++++----------
> 1 file changed, 88 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 99f08e31fd6e0..44cc54a87067a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -969,33 +969,32 @@ mst_connector_atomic_topology_check(struct intel_connector *connector,
> }
>
> static int
> -mst_connector_atomic_check(struct drm_connector *connector,
> +mst_connector_atomic_check(struct drm_connector *_connector,
> struct drm_atomic_state *_state)
> {
> struct intel_atomic_state *state = to_intel_atomic_state(_state);
> - struct intel_connector *intel_connector =
> - to_intel_connector(connector);
> + struct intel_connector *connector = to_intel_connector(_connector);
> int ret;
>
> - ret = intel_digital_connector_atomic_check(connector, &state->base);
> + ret = intel_digital_connector_atomic_check(&connector->base, &state->base);
Clearly a bunch more of our own interfaces should be switched to
intel_connector. But that's for another patch, another time.
> if (ret)
> return ret;
>
> - ret = mst_connector_atomic_topology_check(intel_connector, state);
> + ret = mst_connector_atomic_topology_check(connector, state);
> if (ret)
> return ret;
>
> - if (intel_connector_needs_modeset(state, connector)) {
> + if (intel_connector_needs_modeset(state, &connector->base)) {
> ret = intel_dp_tunnel_atomic_check_state(state,
> - intel_connector->mst_port,
> - intel_connector);
> + connector->mst_port,
> + connector);
> if (ret)
> return ret;
> }
>
> return drm_dp_atomic_release_time_slots(&state->base,
> - &intel_connector->mst_port->mst_mgr,
> - intel_connector->port);
> + &connector->mst_port->mst_mgr,
> + connector->port);
> }
>
> static void mst_stream_disable(struct intel_atomic_state *state,
> @@ -1375,23 +1374,23 @@ static bool mst_stream_initial_fastset_check(struct intel_encoder *encoder,
> return intel_dp_initial_fastset_check(primary_encoder, crtc_state);
> }
>
> -static int mst_connector_get_ddc_modes(struct drm_connector *connector)
> +static int mst_connector_get_ddc_modes(struct drm_connector *_connector)
> {
> - struct intel_display *display = to_intel_display(connector->dev);
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> - struct intel_dp *intel_dp = intel_connector->mst_port;
> + struct intel_connector *connector = to_intel_connector(_connector);
> + struct intel_display *display = to_intel_display(connector->base.dev);
Just to_intel_display(connector) is sufficient.
to_intel_display() intentionally doesn't handle drm_connector, so thus
drm_connector->dev. But intel_connector alone is fine.
> + struct intel_dp *intel_dp = connector->mst_port;
> const struct drm_edid *drm_edid;
> int ret;
>
> - if (drm_connector_is_unregistered(connector))
> - return intel_connector_update_modes(connector, NULL);
> + if (drm_connector_is_unregistered(&connector->base))
> + return intel_connector_update_modes(&connector->base, NULL);
>
> if (!intel_display_driver_check_access(display))
> - return drm_edid_connector_add_modes(connector);
> + return drm_edid_connector_add_modes(&connector->base);
>
> - drm_edid = drm_dp_mst_edid_read(connector, &intel_dp->mst_mgr, intel_connector->port);
> + drm_edid = drm_dp_mst_edid_read(&connector->base, &intel_dp->mst_mgr, connector->port);
>
> - ret = intel_connector_update_modes(connector, drm_edid);
> + ret = intel_connector_update_modes(&connector->base, drm_edid);
>
> drm_edid_free(drm_edid);
>
> @@ -1399,32 +1398,29 @@ static int mst_connector_get_ddc_modes(struct drm_connector *connector)
> }
>
> static int
> -mst_connector_late_register(struct drm_connector *connector)
> +mst_connector_late_register(struct drm_connector *_connector)
> {
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> + struct intel_connector *connector = to_intel_connector(_connector);
> int ret;
>
> - ret = drm_dp_mst_connector_late_register(connector,
> - intel_connector->port);
> + ret = drm_dp_mst_connector_late_register(&connector->base, connector->port);
> if (ret < 0)
> return ret;
>
> - ret = intel_connector_register(connector);
> + ret = intel_connector_register(&connector->base);
> if (ret < 0)
> - drm_dp_mst_connector_early_unregister(connector,
> - intel_connector->port);
> + drm_dp_mst_connector_early_unregister(&connector->base, connector->port);
>
> return ret;
> }
>
> static void
> -mst_connector_early_unregister(struct drm_connector *connector)
> +mst_connector_early_unregister(struct drm_connector *_connector)
> {
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> + struct intel_connector *connector = to_intel_connector(_connector);
>
> - intel_connector_unregister(connector);
> - drm_dp_mst_connector_early_unregister(connector,
> - intel_connector->port);
> + intel_connector_unregister(&connector->base);
> + drm_dp_mst_connector_early_unregister(&connector->base, connector->port);
> }
>
> static const struct drm_connector_funcs mst_connector_funcs = {
> @@ -1438,23 +1434,25 @@ static const struct drm_connector_funcs mst_connector_funcs = {
> .atomic_duplicate_state = intel_digital_connector_duplicate_state,
> };
>
> -static int mst_connector_get_modes(struct drm_connector *connector)
> +static int mst_connector_get_modes(struct drm_connector *_connector)
> {
> - return mst_connector_get_ddc_modes(connector);
> + struct intel_connector *connector = to_intel_connector(_connector);
> +
> + return mst_connector_get_ddc_modes(&connector->base);
> }
>
> static int
> -mst_connector_mode_valid_ctx(struct drm_connector *connector,
> +mst_connector_mode_valid_ctx(struct drm_connector *_connector,
> struct drm_display_mode *mode,
> struct drm_modeset_acquire_ctx *ctx,
> enum drm_mode_status *status)
> {
> - struct intel_display *display = to_intel_display(connector->dev);
> - struct drm_i915_private *dev_priv = to_i915(connector->dev);
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> - struct intel_dp *intel_dp = intel_connector->mst_port;
> + struct intel_connector *connector = to_intel_connector(_connector);
> + struct intel_display *display = to_intel_display(connector->base.dev);
Ditto.
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
Could use display->drm.
> + struct intel_dp *intel_dp = connector->mst_port;
> struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr;
> - struct drm_dp_mst_port *port = intel_connector->port;
> + struct drm_dp_mst_port *port = connector->port;
> const int min_bpp = 18;
> int max_dotclk = display->cdclk.max_dotclk_freq;
> int max_rate, mode_rate, max_lanes, max_link_clock;
> @@ -1465,7 +1463,7 @@ mst_connector_mode_valid_ctx(struct drm_connector *connector,
> int target_clock = mode->clock;
> int num_joined_pipes;
>
> - if (drm_connector_is_unregistered(connector)) {
> + if (drm_connector_is_unregistered(&connector->base)) {
> *status = MODE_ERROR;
> return 0;
> }
> @@ -1503,7 +1501,7 @@ mst_connector_mode_valid_ctx(struct drm_connector *connector,
> * corresponding link capabilities of the sink) in case the
> * stream is uncompressed for it by the last branch device.
> */
> - num_joined_pipes = intel_dp_num_joined_pipes(intel_dp, intel_connector,
> + num_joined_pipes = intel_dp_num_joined_pipes(intel_dp, connector,
> mode->hdisplay, target_clock);
> max_dotclk *= num_joined_pipes;
>
> @@ -1517,14 +1515,14 @@ mst_connector_mode_valid_ctx(struct drm_connector *connector,
> return 0;
> }
>
> - if (intel_dp_has_dsc(intel_connector)) {
> + if (intel_dp_has_dsc(connector)) {
> /*
> * TBD pass the connector BPC,
> * for now U8_MAX so that max BPC on that platform would be picked
> */
> - int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_connector, U8_MAX);
> + int pipe_bpp = intel_dp_dsc_compute_max_bpp(connector, U8_MAX);
>
> - if (drm_dp_sink_supports_fec(intel_connector->dp.fec_capability)) {
> + if (drm_dp_sink_supports_fec(connector->dp.fec_capability)) {
> dsc_max_compressed_bpp =
> intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> max_link_clock,
> @@ -1535,7 +1533,7 @@ mst_connector_mode_valid_ctx(struct drm_connector *connector,
> INTEL_OUTPUT_FORMAT_RGB,
> pipe_bpp, 64);
> dsc_slice_count =
> - intel_dp_dsc_get_slice_count(intel_connector,
> + intel_dp_dsc_get_slice_count(connector,
> target_clock,
> mode->hdisplay,
> num_joined_pipes);
> @@ -1559,39 +1557,39 @@ mst_connector_mode_valid_ctx(struct drm_connector *connector,
> }
>
> static struct drm_encoder *
> -mst_connector_atomic_best_encoder(struct drm_connector *connector,
> +mst_connector_atomic_best_encoder(struct drm_connector *_connector,
> struct drm_atomic_state *state)
> {
> - struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> - connector);
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> - struct intel_dp *intel_dp = intel_connector->mst_port;
> + struct intel_connector *connector = to_intel_connector(_connector);
> + struct drm_connector_state *connector_state =
> + drm_atomic_get_new_connector_state(state, &connector->base);
> + struct intel_dp *intel_dp = connector->mst_port;
> struct intel_crtc *crtc = to_intel_crtc(connector_state->crtc);
>
> return &intel_dp->mst_encoders[crtc->pipe]->base.base;
> }
>
> static int
> -mst_connector_detect_ctx(struct drm_connector *connector,
> +mst_connector_detect_ctx(struct drm_connector *_connector,
> struct drm_modeset_acquire_ctx *ctx, bool force)
> {
> - struct intel_display *display = to_intel_display(connector->dev);
> - struct intel_connector *intel_connector = to_intel_connector(connector);
> - struct intel_dp *intel_dp = intel_connector->mst_port;
> + struct intel_connector *connector = to_intel_connector(_connector);
> + struct intel_display *display = to_intel_display(connector->base.dev);
Ditto.
> + struct intel_dp *intel_dp = connector->mst_port;
>
> if (!intel_display_device_enabled(display))
> return connector_status_disconnected;
>
> - if (drm_connector_is_unregistered(connector))
> + if (drm_connector_is_unregistered(&connector->base))
> return connector_status_disconnected;
>
> if (!intel_display_driver_check_access(display))
> - return connector->status;
> + return connector->base.status;
>
> - intel_dp_flush_connector_commits(intel_connector);
> + intel_dp_flush_connector_commits(connector);
>
> - return drm_dp_mst_detect_port(connector, ctx, &intel_dp->mst_mgr,
> - intel_connector->port);
> + return drm_dp_mst_detect_port(&connector->base, ctx, &intel_dp->mst_mgr,
> + connector->port);
> }
>
> static const struct drm_connector_helper_funcs mst_connector_helper_funcs = {
> @@ -1627,29 +1625,30 @@ static bool mst_connector_get_hw_state(struct intel_connector *connector)
> }
>
> static int mst_topology_add_connector_properties(struct intel_dp *intel_dp,
> - struct drm_connector *connector,
> + struct drm_connector *_connector,
> const char *pathprop)
> {
> struct intel_display *display = to_intel_display(intel_dp);
> + struct intel_connector *connector = to_intel_connector(_connector);
>
> - drm_object_attach_property(&connector->base,
> + drm_object_attach_property(&connector->base.base,
> display->drm->mode_config.path_property, 0);
> - drm_object_attach_property(&connector->base,
> + drm_object_attach_property(&connector->base.base,
> display->drm->mode_config.tile_property, 0);
>
> - intel_attach_force_audio_property(connector);
> - intel_attach_broadcast_rgb_property(connector);
> + intel_attach_force_audio_property(&connector->base);
> + intel_attach_broadcast_rgb_property(&connector->base);
>
> /*
> * Reuse the prop from the SST connector because we're
> * not allowed to create new props after device registration.
> */
> - connector->max_bpc_property =
> + connector->base.max_bpc_property =
> intel_dp->attached_connector->base.max_bpc_property;
> - if (connector->max_bpc_property)
> - drm_connector_attach_max_bpc_property(connector, 6, 12);
> + if (connector->base.max_bpc_property)
> + drm_connector_attach_max_bpc_property(&connector->base, 6, 12);
>
> - return drm_connector_set_path_property(connector, pathprop);
> + return drm_connector_set_path_property(&connector->base, pathprop);
> }
>
> static void
> @@ -1722,62 +1721,59 @@ mst_topology_add_connector(struct drm_dp_mst_topology_mgr *mgr,
> struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr);
> struct intel_display *display = to_intel_display(intel_dp);
> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> - struct intel_connector *intel_connector;
> - struct drm_connector *connector;
> + struct intel_connector *connector;
> enum pipe pipe;
> int ret;
>
> - intel_connector = intel_connector_alloc();
> - if (!intel_connector)
> + connector = intel_connector_alloc();
> + if (!connector)
> return NULL;
>
> - connector = &intel_connector->base;
> -
> - intel_connector->get_hw_state = mst_connector_get_hw_state;
> - intel_connector->sync_state = intel_dp_connector_sync_state;
> - intel_connector->mst_port = intel_dp;
> - intel_connector->port = port;
> + connector->get_hw_state = mst_connector_get_hw_state;
> + connector->sync_state = intel_dp_connector_sync_state;
> + connector->mst_port = intel_dp;
> + connector->port = port;
> drm_dp_mst_get_port_malloc(port);
>
> - intel_dp_init_modeset_retry_work(intel_connector);
> + intel_dp_init_modeset_retry_work(connector);
>
> - ret = drm_connector_dynamic_init(display->drm, connector, &mst_connector_funcs,
> + ret = drm_connector_dynamic_init(display->drm, &connector->base, &mst_connector_funcs,
> DRM_MODE_CONNECTOR_DisplayPort, NULL);
> if (ret)
> goto err_put_port;
>
> - intel_connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port);
> - intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, intel_connector);
> - intel_connector->dp.dsc_hblank_expansion_quirk =
> - detect_dsc_hblank_expansion_quirk(intel_connector);
> + connector->dp.dsc_decompression_aux = drm_dp_mst_dsc_aux_for_port(port);
> + intel_dp_mst_read_decompression_port_dsc_caps(intel_dp, connector);
> + connector->dp.dsc_hblank_expansion_quirk =
> + detect_dsc_hblank_expansion_quirk(connector);
>
> - drm_connector_helper_add(connector, &mst_connector_helper_funcs);
> + drm_connector_helper_add(&connector->base, &mst_connector_helper_funcs);
>
> for_each_pipe(display, pipe) {
> struct drm_encoder *enc =
> &intel_dp->mst_encoders[pipe]->base.base;
>
> - ret = drm_connector_attach_encoder(&intel_connector->base, enc);
> + ret = drm_connector_attach_encoder(&connector->base, enc);
> if (ret)
> goto err_cleanup_connector;
> }
>
> - ret = mst_topology_add_connector_properties(intel_dp, connector, pathprop);
> + ret = mst_topology_add_connector_properties(intel_dp, &connector->base, pathprop);
> if (ret)
> goto err_cleanup_connector;
>
> - ret = intel_dp_hdcp_init(dig_port, intel_connector);
> + ret = intel_dp_hdcp_init(dig_port, connector);
> if (ret)
> drm_dbg_kms(display->drm, "[%s:%d] HDCP MST init failed, skipping.\n",
> - connector->name, connector->base.id);
> + connector->base.name, connector->base.base.id);
>
> - return connector;
> + return &connector->base;
>
> err_cleanup_connector:
> - drm_connector_cleanup(connector);
> + drm_connector_cleanup(&connector->base);
> err_put_port:
> drm_dp_mst_put_port_malloc(port);
> - intel_connector_free(intel_connector);
> + intel_connector_free(connector);
>
> return NULL;
> }
--
Jani Nikula, Intel
next prev parent reply other threads:[~2024-12-12 10:25 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-11 23:03 [PATCH v3 0/7] drm/connector: Expose only a properly inited connector Imre Deak
2024-12-11 23:03 ` [PATCH v3 01/11] drm/connector: Add a way to init/add a connector in separate steps Imre Deak
2024-12-12 0:33 ` Lyude Paul
2024-12-12 10:16 ` Simona Vetter
2024-12-12 10:04 ` Jani Nikula
2024-12-12 11:45 ` Imre Deak
2024-12-16 12:22 ` Maxime Ripard
2024-12-11 23:03 ` [PATCH v3 02/11] drm/connector: Add FIXME for GETRESOURCES ioctl wrt. uninited connectors Imre Deak
2024-12-12 10:06 ` Jani Nikula
2024-12-13 12:02 ` Imre Deak
2024-12-11 23:03 ` [PATCH v3 03/11] drm/connector: Add deprication notes for drm_connector_register/unregister Imre Deak
2024-12-12 10:10 ` Jani Nikula
2024-12-12 12:02 ` Imre Deak
2024-12-16 14:37 ` Simona Vetter
2024-12-11 23:03 ` [PATCH v3 04/11] drm/dp_mst: Register connectors via drm_connector_dynamic_register() Imre Deak
2024-12-12 10:12 ` Jani Nikula
2024-12-13 12:06 ` Imre Deak
2024-12-16 11:03 ` Jani Nikula
2024-12-16 12:14 ` Imre Deak
2024-12-16 12:23 ` Imre Deak
2024-12-17 8:02 ` Lin, Wayne
2024-12-11 23:03 ` [PATCH v3 05/11] drm/i915/dp_mst: Expose a connector to kernel users after it's properly initialized Imre Deak
2024-12-12 10:13 ` Jani Nikula
2024-12-11 23:03 ` [PATCH v3 06/11] drm/amd/dp_mst: " Imre Deak
2024-12-12 10:14 ` Jani Nikula
2024-12-16 12:45 ` Imre Deak
2024-12-16 18:26 ` Alex Deucher
2024-12-11 23:03 ` [PATCH v3 07/11] drm/nouveau/dp_mst: " Imre Deak
2024-12-12 10:14 ` Jani Nikula
2024-12-11 23:03 ` [PATCH v3 08/11] drm/connector: Warn if a connector is registered/added incorrectly Imre Deak
2024-12-12 10:15 ` Jani Nikula
2024-12-11 23:03 ` [PATCH v3 09/11] drm/tests: Add tests for drm_connector_dynamic_init()/register() Imre Deak
2024-12-16 12:22 ` Maxime Ripard
2024-12-11 23:03 ` [PATCH v3 10/11] drm/i915/dp_mst: Fix error handling while adding a connector Imre Deak
2024-12-12 10:16 ` Jani Nikula
2024-12-11 23:03 ` [PATCH v3 11/11] drm/i915/dp_mst: Use intel_connector vs. drm_connector pointer in intel_dp_mst.c Imre Deak
2024-12-12 10:25 ` Jani Nikula [this message]
2024-12-12 12:23 ` Imre Deak
2024-12-11 23:33 ` ✗ Fi.CI.CHECKPATCH: warning for drm/connector: Expose only a properly inited connector Patchwork
2024-12-11 23:33 ` ✗ Fi.CI.SPARSE: " Patchwork
2024-12-11 23:57 ` ✓ i915.CI.BAT: success " Patchwork
2024-12-12 7:33 ` ✗ i915.CI.Full: failure " Patchwork
2024-12-13 15:41 ` Imre Deak
2024-12-16 12:22 ` [PATCH v3 0/7] " Imre Deak
2024-12-16 12:39 ` Maxime Ripard
2024-12-17 14:47 ` Imre Deak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ttb9w63h.fsf@intel.com \
--to=jani.nikula@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.