On Tue, 06 Jan 2026, Dibin Moolakadan Subrahmanian
<[email protected]> wrote:
> On 05-01-2026 18:25, Jani Nikula wrote:
>> On Tue, 09 Dec 2025, Dibin Moolakadan
>> Subrahmanian<[email protected]> wrote:
>>> Introduce dc3co_allow in struct intel_display and determine DC3CO
>>> eligibility during atomic_check(). DC3CO is permitted only when:
>>>
>>> - the active pipe drives eDP,
>>> - the pipe is single-pipe (no joiner),
>>> - the pipe/port combination supports DC3CO.
>>>
>>> When eligible, intel_atomic_commit_tail() programs the target DC state
>>> as DC_STATE_EN_UPTO_DC3CO; otherwise we fall back to DC6. Update the
>>> PSR vblank enable/disable path to follow the same policy.
>>>
>>> Also extend get_allowed_dc_mask() to expose DC3CO support on
>>> DISPLAY_VER >= 35.
>>>
>>> Signed-off-by: Dibin Moolakadan
>>> Subrahmanian<[email protected]>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_display.c | 75 +++++++++++++++++++
>>> drivers/gpu/drm/i915/display/intel_display.h | 1 +
>>> .../gpu/drm/i915/display/intel_display_core.h | 3 +
>>> .../drm/i915/display/intel_display_power.c | 4 +-
>>> drivers/gpu/drm/i915/display/intel_psr.c | 13 ++--
>>> 5 files changed, 87 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>>> b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 9c6d3ecdb589..205f55a87736 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -6295,6 +6295,75 @@ static int intel_joiner_add_affected_crtcs(struct
>>> intel_atomic_state *state)
>>> return 0;
>>> }
>>>
>>> +bool intel_dc3co_allowed(struct intel_display *display)
>>> +{
>>> + return display->power.dc3co_allow;
>> Very few files should touch display->power, and this is not one of them.
>>
>> 'git grep "display->power" -- drivers/gpu/drm/i915/display'
>
> Yes, git grep shows few files , I will try to move all dc3co functions to
> drivers/gpu/drm/i915/display/intel_display_power.c.
>
>>
>> When is it okay to call this function and expect to get sane results?
>
> display->power.dc3co_allow is only updated in intel_dc3co_allow_check() which
> is called from
> intel_atomic_commit_tail().intel_dc3co_allowed() only intended to be called
> from intel_post_plane_update()
> path(ALPM/PSR), which executes as part of intel_atomic_commit_tail().
The point being, you have a complicated set of rules, and who's going to
know them and how when reading the code?
BR,
Jani.
>
>>
>>> +}
>>> +
>>> +static bool intel_dc3co_port_pipe_compatible(struct intel_dp *intel_dp,
>>> + const struct intel_crtc_state
>>> *crtc_state)
>>> +{
>>> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> + enum pipe pipe = to_intel_crtc(crtc_state->uapi.crtc)->pipe;
>>> + enum port port = dig_port->base.port;
>>> + int num_pipes = intel_crtc_num_joined_pipes(crtc_state);
>>> +
>>> + if (num_pipes != 1)
>>> + return false;
>>> +
>>> + if (!(pipe <= PIPE_B && port <= PORT_B))
>>> + return false;
>>> +
>>> + return true;
>> That's a really complicated way to say
>>
>> return num_pipes == 1 && pipe <= PIPEB && port <= PORT_B;
>
> I will update this.
>
>>
>>> +}
>>> +
>>> +static void intel_dc3co_allow_check(struct intel_atomic_state *state)
>> What does "check" mean here? Or in *any* function?
>>
>> Check sounds like something that's a pure function that doesn't change
>> anything... but this does.
>
> I will split this function to two , one for check and one for initialization
>
>>> +{
>>> + struct intel_display *display = to_intel_display(state);
>>> + struct intel_crtc *crtc;
>>> + struct intel_crtc_state *new_crtc_state;
>>> + struct intel_encoder *encoder;
>>> + struct intel_dp *intel_dp;
>>> + int i;
>>> + struct i915_power_domains *power_domains = &display->power.domains;
>>> + bool any_active = false;
>>> + bool allow = true;
>>> +
>>> + display->power.dc3co_allow = 0;
>> That's now cached state with no stated rules on when it's valid and when
>> it's not.
>>
>>> +
>>> + if ((power_domains->allowed_dc_mask & DC_STATE_EN_UPTO_DC3CO) !=
>>> DC_STATE_EN_UPTO_DC3CO)
>>> + return;
>>> +
>>> + for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>> + if (!new_crtc_state->hw.active)
>>> + continue;
>>> +
>>> + any_active = true;
>>> +
>>> + for_each_intel_encoder_mask(display->drm, encoder,
>>> + new_crtc_state->uapi.encoder_mask) {
>>> + /* If any active pipe not eDP disable*/
>> What?
>
> I will correct comment /* Disallow DC3CO if any active pipe is not eDP */
>
>>
>>> + if (!intel_encoder_is_dp(encoder) ||
>>> + encoder->type != INTEL_OUTPUT_EDP) {
>>> + allow = false;
>>> + goto out;
>>> + }
>>> + intel_dp = enc_to_intel_dp(encoder);
>>> + /* Port, joiner, pipe placement checks */
>> Is that a helpful comment?
>>
>>> + if (!intel_dc3co_port_pipe_compatible(intel_dp,
>>> new_crtc_state)) {
>>> + allow = false;
>>> + goto out;
>>> + }
>>> + }
>>> + }
>>> +
>>> + if (!any_active)
>>> + allow = false;
>>> +
>>> +out:
>>> + display->power.dc3co_allow = allow;
>>> +}
>>> +
>> intel_display.[ch] is not the dumping ground for random new code. The
>> goal is to *reduce* the size of it, not increase.
>
> This function needs encoder,port and pipe information, which is why I added
> it in intel_display.c.However,I agree it is updating dc3co_allow . I will
> check if
> it can be moved to intel_display_power.c.
>
>>
>>> static int intel_atomic_check_config(struct intel_atomic_state *state,
>>> struct intel_link_bw_limits *limits,
>>> enum pipe *failed_pipe)
>>> @@ -6565,6 +6634,8 @@ int intel_atomic_check(struct drm_device *dev,
>>> if (ret)
>>> goto fail;
>>>
>>> + intel_dc3co_allow_check(state);
>>> +
>>> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>>> new_crtc_state, i) {
>>> intel_color_assert_luts(new_crtc_state);
>>> @@ -7601,6 +7672,10 @@ static void intel_atomic_commit_tail(struct
>>> intel_atomic_state *state)
>>> */
>>> intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore);
>>> }
>>> + if (intel_dc3co_allowed(display))
>>> + intel_display_power_set_target_dc_state(display,
>>> DC_STATE_EN_UPTO_DC3CO);
>>> + else
>>> + intel_display_power_set_target_dc_state(display,
>>> DC_STATE_EN_UPTO_DC6);
>>> /*
>>> * Delay re-enabling DC states by 17 ms to avoid the off->on->off
>>> * toggling overhead at and above 60 FPS.
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
>>> b/drivers/gpu/drm/i915/display/intel_display.h
>>> index f8e6e4e82722..97987f082560 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.h
>>> @@ -560,5 +560,6 @@ bool assert_port_valid(struct intel_display *display,
>>> enum port port);
>>>
>>> bool intel_scanout_needs_vtd_wa(struct intel_display *display);
>>> int intel_crtc_num_joined_pipes(const struct intel_crtc_state
>>> *crtc_state);
>>> +bool intel_dc3co_allowed(struct intel_display *display);
>>>
>>> #endif
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
>>> b/drivers/gpu/drm/i915/display/intel_display_core.h
>>> index d708d322aa85..fa567c95029c 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>>> @@ -538,6 +538,9 @@ struct intel_display {
>>>
>>> /* perform PHY state sanity checks? */
>>> bool chv_phy_assert[2];
>>> +
>>> + /* mark dc3co entry is allowed*/
>> ^- space missing
> will add space.
>>
>>> + bool dc3co_allow;
>> Still unclear when this is valid.
>>
>>> } power;
>>>
>>> struct {
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
>>> b/drivers/gpu/drm/i915/display/intel_display_power.c
>>> index 0961b194554c..e99552f18756 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>>> @@ -956,7 +956,9 @@ static u32 get_allowed_dc_mask(struct intel_display
>>> *display, int enable_dc)
>>> if (!HAS_DISPLAY(display))
>>> return 0;
>>>
>>> - if (DISPLAY_VER(display) >= 20)
>>> + if (DISPLAY_VER(display) >= 35)
>>> + max_dc = 3;
>>> + else if (DISPLAY_VER(display) >= 20)
>>> max_dc = 2;
>>> else if (display->platform.dg2)
>>> max_dc = 1;
>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>>> b/drivers/gpu/drm/i915/display/intel_psr.c
>>> index 753359069044..9c616f449ad6 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>> @@ -3903,14 +3903,11 @@ void intel_psr_notify_vblank_enable_disable(struct
>>> intel_display *display,
>>> return;
>>> }
>>>
>>> - /*
>>> - * NOTE: intel_display_power_set_target_dc_state is used
>>> - * only by PSR * code for DC3CO handling. DC3CO target
>>> - * state is currently disabled in * PSR code. If DC3CO
>>> - * is taken into use we need take that into account here
>>> - * as well.
>>> - */
>>> - intel_display_power_set_target_dc_state(display, enable ?
>>> DC_STATE_DISABLE :
>>> + if (intel_dc3co_allowed(display))
>>> + intel_display_power_set_target_dc_state(display, enable ?
>>> DC_STATE_DISABLE :
>>> + DC_STATE_EN_UPTO_DC3CO);
>>> + else
>>> + intel_display_power_set_target_dc_state(display, enable ?
>>> DC_STATE_DISABLE :
>>> DC_STATE_EN_UPTO_DC6);
>>> }
--
Jani Nikula, Intel
