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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).