From: "Murthy, Arun R" <arun.r.murthy@intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>,
Simona Vetter <simona@ffwll.ch>, <ville.syrjala@linux.intel.com>,
<suraj.kandpal@intel.com>, <imre.deak@intel.com>
Cc: <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
<intel-gfx@lists.freedesktop.org>,
<intel-xe@lists.freedesktop.org>
Subject: Re: [PATCH RFC 3/3] drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx
Date: Mon, 9 Mar 2026 18:50:21 +0530 [thread overview]
Message-ID: <9faf7059-b9c1-4de5-ab58-a04d80b4b1b6@intel.com> (raw)
In-Reply-To: <63a9df004fb9843a54b8c7702102d2dc0cc3d948@intel.com>
On 09-03-2026 18:39, Jani Nikula wrote:
> On Mon, 09 Mar 2026, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> Use re_rmw and update the required bits for PORT_AUX_CTL and drop the
>> bit configurations that are not required(AUX Power Request setting of
>> bit 19). Also break writing to PORT_AUX_CTL into 2 steps with first step
>> for doing the configuration/settings and then second write to trigger
>> the AUX transaction.
> The primary question the commit message should answer is, "Why?"
>
> There's a whole lot of "What?" here, indeed too much since the patch is
> doing too many things in one go.
>
> The point of an RFC patch is to solicit feedback on the idea. But the
> idea remains vague here as there's no rationale why this is needed.
Before initiating the AUX transaction, power well is enabled by calling
intel_display_power_get().
Here the AUX Power Request bit19 of PORT_AUX_CTL register is being set
and with a timeout of 600us the AUX Power State is checked to see if the
power is enabled.
Then as part of the AUX transaction, PORT_AUX_CTL is written using
intel_reg_write. Since we are writing to the entire register the same
bit19 AUX Power Request is again being set to ensure the bit is set.
This setting of bit is un-necessary as its already being set in
intel_display_power_get.
Hence in order to overcome this un-necessary setting of bit,
read/mask/write is being used.
Thanks and Regards,
Arun R Murthy
-------------------
>
> BR,
> Jani.
>
>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_display_types.h | 6 +-
>> drivers/gpu/drm/i915/display/intel_dp_aux.c | 83 ++++++++++++++--------
>> drivers/gpu/drm/i915/display/intel_psr.c | 29 +++++---
>> 3 files changed, 76 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index e189f8c39ccb440f99cd642de177b18f3b605753..341749452579acfc3e08715d2f0b211bf6489dd9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -1882,10 +1882,10 @@ struct intel_dp {
>>
>> u32 (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>> /*
>> - * This function returns the value we have to program the AUX_CTL
>> - * register with to kick off an AUX transaction.
>> + * This function programs the configuration/settings for the AUX_CTL
>> + * register but dont kick off an AUX transaction.
>> */
>> - u32 (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>> + void (*get_aux_send_ctl)(struct intel_dp *dp, int send_bytes,
>> u32 aux_clock_divider);
>>
>> i915_reg_t (*aux_ch_ctl_reg)(struct intel_dp *dp);
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> index 0a9e2d6cdbc5d9e0d17b2db60a32cf20a3bad6b6..4fef378e0a8fbf79211fd98913e507e90b2b48ea 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
>> @@ -175,12 +175,13 @@ static int g4x_dp_aux_precharge_len(void)
>> precharge_min - preamble) / 2;
>> }
>>
>> -static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> - int send_bytes,
>> - u32 aux_clock_divider)
>> +static void g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> + int send_bytes,
>> + u32 aux_clock_divider)
>> {
>> struct intel_display *display = to_intel_display(intel_dp);
>> - u32 timeout;
>> + i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>> + u32 timeout, value;
>>
>> /* Max timeout value on G4x-BDW: 1.6ms */
>> if (display->platform.broadwell)
>> @@ -188,8 +189,7 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> else
>> timeout = DP_AUX_CH_CTL_TIME_OUT_400us;
>>
>> - return DP_AUX_CH_CTL_SEND_BUSY |
>> - DP_AUX_CH_CTL_DONE |
>> + value = DP_AUX_CH_CTL_DONE |
>> DP_AUX_CH_CTL_INTERRUPT |
>> DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> timeout |
>> @@ -197,23 +197,35 @@ static u32 g4x_get_aux_send_ctl(struct intel_dp *intel_dp,
>> DP_AUX_CH_CTL_MESSAGE_SIZE(send_bytes) |
>> DP_AUX_CH_CTL_PRECHARGE_2US(g4x_dp_aux_precharge_len()) |
>> DP_AUX_CH_CTL_BIT_CLOCK_2X(aux_clock_divider);
>> +
>> + intel_de_rmw(display, ch_ctl,
>> + (DP_AUX_CH_CTL_DONE |
>> + DP_AUX_CH_CTL_INTERRUPT |
>> + DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> + DP_AUX_CH_CTL_TIME_OUT_MASK |
>> + DP_AUX_CH_CTL_RECEIVE_ERROR |
>> + DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
>> + DP_AUX_CH_CTL_PRECHARGE_2US_MASK |
>> + DP_AUX_CH_CTL_BIT_CLOCK_2X_MASK),
>> + value);
>> + return;
>> }
>>
>> -static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> - int send_bytes,
>> - u32 unused)
>> +static void skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> + int send_bytes,
>> + u32 unused)
>> {
>> struct intel_display *display = to_intel_display(intel_dp);
>> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> - u32 ret;
>> + i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp);
>> + u32 value;
>>
>> /*
>> * Max timeout values:
>> * SKL-GLK: 1.6ms
>> * ICL+: 4ms
>> */
>> - ret = DP_AUX_CH_CTL_SEND_BUSY |
>> - DP_AUX_CH_CTL_DONE |
>> + value = DP_AUX_CH_CTL_DONE |
>> DP_AUX_CH_CTL_INTERRUPT |
>> DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> DP_AUX_CH_CTL_TIME_OUT_MAX |
>> @@ -222,17 +234,22 @@ static u32 skl_get_aux_send_ctl(struct intel_dp *intel_dp,
>> DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len(intel_dp)) |
>> DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len());
>>
>> - if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> - ret |= DP_AUX_CH_CTL_TBT_IO;
>> + intel_de_rmw(display, ch_ctl,
>> + (DP_AUX_CH_CTL_DONE |
>> + DP_AUX_CH_CTL_INTERRUPT |
>> + DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> + DP_AUX_CH_CTL_TIME_OUT_MASK |
>> + DP_AUX_CH_CTL_RECEIVE_ERROR |
>> + DP_AUX_CH_CTL_MESSAGE_SIZE_MASK |
>> + DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL_MASK |
>> + DP_AUX_CH_CTL_SYNC_PULSE_SKL_MASK),
>> + value);
>>
>> - /*
>> - * Power request bit is already set during aux power well enable.
>> - * Preserve the bit across aux transactions.
>> - */
>> - if (DISPLAY_VER(display) >= 14)
>> - ret |= XELPDP_DP_AUX_CH_CTL_POWER_REQUEST;
>> + if (intel_tc_port_in_tbt_alt_mode(dig_port))
>> + intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_TBT_IO,
>> + DP_AUX_CH_CTL_TBT_IO);
>>
>> - return ret;
>> + return;
>> }
>>
>> static int
>> @@ -341,11 +358,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>> }
>>
>> while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
>> - u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
>> - send_bytes,
>> - aux_clock_divider);
>> + intel_dp->get_aux_send_ctl(intel_dp, send_bytes,
>> + aux_clock_divider);
>>
>> - send_ctl |= aux_send_ctl_flags;
>> + /* Update the flags */
>> + intel_de_rmw(display, ch_ctl, DP_AUX_CH_CTL_AUX_AKSV_SELECT,
>> + aux_send_ctl_flags);
>>
>> /* Must try at least 3 times according to DP spec */
>> for (try = 0; try < 5; try++) {
>> @@ -356,15 +374,20 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>> send_bytes - i));
>>
>> /* Send the command and wait for it to complete */
>> - intel_de_write(display, ch_ctl, send_ctl);
>> + intel_de_rmw(display, ch_ctl,
>> + DP_AUX_CH_CTL_SEND_BUSY,
>> + DP_AUX_CH_CTL_SEND_BUSY);
>>
>> status = intel_dp_aux_wait_done(intel_dp);
>>
>> /* Clear done status and any errors */
>> - intel_de_write(display, ch_ctl,
>> - status | DP_AUX_CH_CTL_DONE |
>> - DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> - DP_AUX_CH_CTL_RECEIVE_ERROR);
>> + intel_de_rmw(display, ch_ctl,
>> + (DP_AUX_CH_CTL_DONE |
>> + DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> + DP_AUX_CH_CTL_RECEIVE_ERROR),
>> + (DP_AUX_CH_CTL_DONE |
>> + DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> + DP_AUX_CH_CTL_RECEIVE_ERROR));
>>
>> /*
>> * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 9296ca3a4ff468a6e61babd81217e4ba19b15062..e06e04f20355d511e5c58fc28866aa763fd65a4b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -722,7 +722,9 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>> {
>> struct intel_display *display = to_intel_display(intel_dp);
>> enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> + i915_reg_t ch_ctl = psr_aux_ctl_reg(display, cpu_transcoder);
>> u32 aux_clock_divider, aux_ctl;
>> +
>> /* write DP_SET_POWER=D0 */
>> static const u8 aux_msg[] = {
>> [0] = (DP_AUX_NATIVE_WRITE << 4) | ((DP_SET_POWER >> 16) & 0xf),
>> @@ -742,17 +744,26 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>> aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>>
>> /* Start with bits set for DDI_AUX_CTL register */
>> - aux_ctl = intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>> - aux_clock_divider);
>> + intel_dp->get_aux_send_ctl(intel_dp, sizeof(aux_msg),
>> + aux_clock_divider);
>>
>> /* Select only valid bits for SRD_AUX_CTL */
>> - aux_ctl &= EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> - EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> - EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> - EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
>> -
>> - intel_de_write(display, psr_aux_ctl_reg(display, cpu_transcoder),
>> - aux_ctl);
>> + aux_ctl = EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> + EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> + EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> + EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
>> +
>> + intel_de_rmw(display, ch_ctl,
>> + (EDP_PSR_AUX_CTL_TIME_OUT_MASK |
>> + EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
>> + EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
>> + EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK),
>> + aux_ctl);
>> +
>> + /* Send the command or intitate the AUX transaction */
>> + intel_de_rmw(display, ch_ctl,
>> + DP_AUX_CH_CTL_SEND_BUSY,
>> + DP_AUX_CH_CTL_SEND_BUSY);
>> }
>>
>> static bool psr2_su_region_et_valid(struct intel_connector *connector, bool panel_replay)
next prev parent reply other threads:[~2026-03-09 13:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-09 7:29 [PATCH RFC 0/3] Some updates over DP AUX Transactions Arun R Murthy
2026-03-09 7:29 ` [PATCH RFC 1/3] drm/display/dp: Export function to wake the sink AUX_CH Arun R Murthy
2026-03-10 2:41 ` Claude review: " Claude Code Review Bot
2026-03-09 7:29 ` [PATCH RFC 2/3] drm/i915/dp: On AUX_CH tx timeout, wake up the sink Arun R Murthy
2026-03-10 2:41 ` Claude review: " Claude Code Review Bot
2026-03-09 7:29 ` [PATCH RFC 3/3] drm/i915/dp: Configure PORT_AUX_CTL and then trigger the tx Arun R Murthy
2026-03-09 13:09 ` Jani Nikula
2026-03-09 13:20 ` Murthy, Arun R [this message]
2026-03-10 2:41 ` Claude review: " Claude Code Review Bot
2026-03-10 2:41 ` Claude review: Some updates over DP AUX Transactions Claude Code Review Bot
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=9faf7059-b9c1-4de5-ab58-a04d80b4b1b6@intel.com \
--to=arun.r.murthy@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=imre.deak@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=simona@ffwll.ch \
--cc=suraj.kandpal@intel.com \
--cc=ville.syrjala@linux.intel.com \
/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