RE: [PATCH v4 02/10] drm/i915/alpm: alpm_init() for DP2.1

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 18 Nov 2025, "Manna, Animesh" <animesh.manna@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Monday, November 17, 2025 8:59 PM
>> To: Manna, Animesh <animesh.manna@intel.com>; intel-
>> gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org; dri-
>> devel@lists.freedesktop.org
>> Cc: Kandpal, Suraj <suraj.kandpal@intel.com>; Manna, Animesh
>> <animesh.manna@intel.com>; Hogander, Jouni
>> <jouni.hogander@intel.com>
>> Subject: Re: [PATCH v4 02/10] drm/i915/alpm: alpm_init() for DP2.1
>> 
>> On Thu, 13 Nov 2025, Animesh Manna <animesh.manna@intel.com> 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 <jouni.hogander@intel.com>
>> > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> > ---
>> >  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