On Tue, 18 Nov 2025, "Manna, Animesh" <[email protected]> wrote: >> -----Original Message----- >> From: Nikula, Jani <[email protected]> >> Sent: Monday, November 17, 2025 8:59 PM >> To: Manna, Animesh <[email protected]>; intel- >> [email protected]; [email protected]; dri- >> [email protected] >> Cc: Kandpal, Suraj <[email protected]>; Manna, Animesh >> <[email protected]>; Hogander, Jouni >> <[email protected]> >> Subject: Re: [PATCH v4 02/10] drm/i915/alpm: alpm_init() for DP2.1 >> >> On Thu, 13 Nov 2025, Animesh Manna <[email protected]> wrote: >> > Initialize ALPM for DP2.1 and separate out ALPM mutex-init from >> > alpm-init. >> >> I thought I said you're going to need multiple init functions. Don't move the >> alpm mutex init away from alpm code. It needs to stay in alpm code. > > Only for mutex-init do you want me to add a separate function?
Yes, I absolutely do. It's not about how small or big something is, it's about the organization of the code. All things alpm should be in intel_alpm.[ch]. We also have alpm sub-struct in struct intel_dp; as a rule of thumb only intel_alpm.[ch] should touch stuff in that sub-struct. This also sets the example of what to do next, and where to add stuff next. If you have the mutex init in intel_dp.c, if more init is needed later, it'll also get added in intel_dp.c, intead of intel_alpm.c where it belongs. >> And now the whole patch and subject and commit message talk of >> completely different things. > > Earlier alpm is initialized for EDP only, now its extended for DP2.1 in this > patch. > Earlier mutex init is part of alpm-init. Now after feedback I also felt it > can be separate out because reading dpcd always not possible if the display > is disconnected for dp-connector but mutex-init can be done. > So mentioned as separate out ALPM mutex-init from alpm-init(). > >> >> Please read the review comments, and ask questions if they comments are >> not clear. > > Currently I am little confused, need change in code or commit description. > From here onwards if you can point out some specific change will modify > accordingly in next version. As the first thing, I think you should add a function such as intel_alpm_init_dpcd(), and move the DPCD parts of intel_alpm_init() there. The mutex init should remain in intel_alpm_init(). intel_alpm_init_dpcd() should be called from intel_edp_init_dpcd(). That's it, that's the first thing. Don't add anything more. The commit message should of course describe those changes. It should be all non-functional, no addition of new platform checks or anything. The next part would be moving intel_alpm_init() to be done for all DP, not just eDP, and calling intel_alpm_init_dpcd() also from intel_dp_detect_dpcd(), where supported. Small incremental things, one thing at a time. BR, Jani. > > Regards, > Animesh > >> >> BR, >> Jani. >> >> >> > >> > v2: Separate out mutex-init. [Jani] >> > v3: Refactor further to avoid DISPLAY_VER check in multiple places. >> > [Jani] >> > V4: Cosmetic changes. [Suraj] >> > >> > Cc: Jouni Högander <[email protected]> >> > Reviewed-by: Suraj Kandpal <[email protected]> >> > Signed-off-by: Animesh Manna <[email protected]> >> > --- >> > drivers/gpu/drm/i915/display/intel_alpm.c | 16 ++++++++++++++-- >> > drivers/gpu/drm/i915/display/intel_alpm.h | 3 ++- >> > drivers/gpu/drm/i915/display/intel_dp.c | 8 +++++++- >> > 3 files changed, 23 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c >> > b/drivers/gpu/drm/i915/display/intel_alpm.c >> > index 6372f533f65b..14acd6717e59 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_alpm.c >> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c >> > @@ -41,7 +41,20 @@ bool intel_alpm_is_alpm_aux_less(struct intel_dp >> *intel_dp, >> > (crtc_state->has_lobf && >> > intel_alpm_aux_less_wake_supported(intel_dp)); >> > } >> > >> > -void intel_alpm_init(struct intel_dp *intel_dp) >> > +bool intel_alpm_source_supported(struct intel_connector *connector) { >> > + struct intel_display *display = to_intel_display(connector); >> > + >> > + if (!((connector->base.connector_type == >> DRM_MODE_CONNECTOR_DisplayPort && >> > + DISPLAY_VER(display) >= 35) || >> > + (connector->base.connector_type == >> DRM_MODE_CONNECTOR_eDP && >> > + DISPLAY_VER(display) >= 20))) >> > + return false; >> > + >> > + return true; >> > +} >> > + >> > +void intel_alpm_get_sink_capability(struct intel_dp *intel_dp) >> > { >> > u8 dpcd; >> > >> > @@ -49,7 +62,6 @@ void intel_alpm_init(struct intel_dp *intel_dp) >> > return; >> > >> > intel_dp->alpm_dpcd = dpcd; >> > - mutex_init(&intel_dp->alpm.lock); >> > } >> > >> > static int get_silence_period_symbols(const struct intel_crtc_state >> > *crtc_state) diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h >> > b/drivers/gpu/drm/i915/display/intel_alpm.h >> > index 53599b464dea..bcc354a46a1d 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_alpm.h >> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h >> > @@ -15,7 +15,8 @@ struct intel_connector; struct intel_atomic_state; >> > struct intel_crtc; >> > >> > -void intel_alpm_init(struct intel_dp *intel_dp); >> > +bool intel_alpm_source_supported(struct intel_connector *connector); >> > +void intel_alpm_get_sink_capability(struct intel_dp *intel_dp); >> > bool intel_alpm_compute_params(struct intel_dp *intel_dp, >> > struct intel_crtc_state *crtc_state); void >> > intel_alpm_lobf_compute_config(struct intel_dp *intel_dp, diff --git >> > a/drivers/gpu/drm/i915/display/intel_dp.c >> > b/drivers/gpu/drm/i915/display/intel_dp.c >> > index 0ec82fcbcf48..81dd5bf7e3c5 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> > @@ -6074,6 +6074,9 @@ intel_dp_detect(struct drm_connector >> *_connector, >> > if (ret == 1) >> > connector->base.epoch_counter++; >> > >> > + if (intel_alpm_source_supported(connector)) >> > + intel_alpm_get_sink_capability(intel_dp); >> > + >> > if (!intel_dp_is_edp(intel_dp)) >> > intel_psr_init_dpcd(intel_dp); >> > >> > @@ -6716,7 +6719,7 @@ static bool intel_edp_init_connector(struct >> intel_dp *intel_dp, >> > */ >> > intel_hpd_enable_detection(encoder); >> > >> > - intel_alpm_init(intel_dp); >> > + intel_alpm_get_sink_capability(intel_dp); >> > >> > /* Cache DPCD and EDID for edp. */ >> > has_dpcd = intel_edp_init_dpcd(intel_dp, connector); @@ -6932,6 >> > +6935,9 @@ intel_dp_init_connector(struct intel_digital_port >> > *dig_port, >> > >> > intel_psr_init(intel_dp); >> > >> > + if (intel_alpm_source_supported(connector)) >> > + mutex_init(&intel_dp->alpm.lock); >> > + >> > return true; >> > >> > fail: >> >> -- >> Jani Nikula, Intel -- Jani Nikula, Intel
