On Thu, Dec 12, 2024 at 12:25:38PM +0200, Jani Nikula wrote:
> On Thu, 12 Dec 2024, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> A few issues below that should be addressed, with those fixed,
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> > ---
> > 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.
Agreed with this and the rest later. They are however separate changes
than what's the commit describes. For both author and reviewer it would
be better to do these in separate patches (even though it's good that
you noticed this now). Are you ok with that?
> > + 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
