Quoting Jani Nikula (2025-01-03 08:11:59-03:00)
>On Tue, 31 Dec 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>> Quoting Jani Nikula (2024-12-31 08:45:56-03:00)
>>>On Tue, 24 Dec 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>>>I understand you're following patterns from elsewhere in the driver. But
>>
>> Correct.
>>
>>>I've always wondered why we use a mixture of atomic state, global state,
>>>and the specific (e.g. struct intel_cmtg_state) here. Makes no sense.
>>>
>>>I believe the specific global state structs should all be internal to
>>>the implementation in the .c file, opaque outside, with accessor
>>>functions. The to_intel_cmtg_state() should be a proper function
>>>(although the constness handling may require a _Generic wrapper).
>>
>> Yeah. I agree that the specific state bits should be private to the
>> implementation. Even the type "struct intel_cmtg_state" could be private
>> and then we would have the exposed interface dealing with only "struct
>> intel_global_state" or "struct intel_atomic_state" pointers.
>>
>> The only function that is currently asking for a struct intel_cmtg_state
>> pointer is intel_cmtg_readout_state(), but that one can be easily
>> changed to receive a pointer to struct intel_global_state instead (or
>> even converted to be a function specific to display->cmtg.obj.state,
>> dropping the state argument).
>>
>> With that, there would be no need to expose the struct intel_cmtg_state
>> type anymore and it can be made entirely private to intel_cmtg.c.
>>
>> Let me know what you think.
>>
>>>
>>>I actually have had patches to do this for all the global state stuff,
>>>but they've conflicted and gone stale. It's hard when basically anyone
>>>can just poke at the state when this shouldn't really be the case.
>>
>> We could maybe start with CMTG state and progressively converting the
>> other guys?
>
>I kind of jumped the gun with pmdemand that you already reviewed. That
>could be the minimal direction here too. There's the to_intel_*_state()
>but it could be an intermediate step towards the right direction. Could
>do that here as well.
>
>> (Although, after reading the entire review, if we decide to deal with
>> the CMTG only as part of the sanitization, I guess implementing the
>> whole global state "protocol" for the CMTG wouldn't make much sense
>> anymore, right?).
>
>I'll reply to this below.
>
>>
>>>
>>>
>>>> +
>>>> +#endif /* __INTEL_CMTG_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_cmtg_regs.h b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>>> new file mode 100644
>>>> index 000000000000..082f96cad284
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/display/intel_cmtg_regs.h
>>>> @@ -0,0 +1,21 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright (C) 2024 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef __INTEL_CMTG_REGS_H__
>>>> +#define __INTEL_CMTG_REGS_H__
>>>> +
>>>> +#include "i915_reg_defs.h"
>>>> +
>>>> +#define CMTG_CLK_SEL _MMIO(0x46160)
>>>> +#define CMTG_CLK_SEL_A_MASK REG_GENMASK(31, 29)
>>>> +#define CMTG_CLK_SEL_A_DISABLED REG_FIELD_PREP(CMTG_CLK_SEL_A_MASK, 0)
>>>> +#define CMTG_CLK_SEL_B_MASK REG_GENMASK(15, 13)
>>>> +#define CMTG_CLK_SEL_B_DISABLED REG_FIELD_PREP(CMTG_CLK_SEL_B_MASK, 0)
>>>> +
>>>> +#define TRANS_CMTG_CTL_A _MMIO(0x6fa88)
>>>> +#define TRANS_CMTG_CTL_B _MMIO(0x6fb88)
>>>
>>>Could make those underscore prefixed, with a parametrized
>>>TRANS_CMTG_CTL(idx).
>>
>> I had thought about that, but then decided to go with two separate
>> defines.
>>
>> The main reason was because of the fact that the second instance was
>> added to support the async dual eDP case and not necessarily to be a
>> common "per pipe" resource like with other pipe/transcoder components.
>
>Not insisting, not a huge deal.
>
>>
>>>
>>>> +#define CMTG_ENABLE REG_BIT(31)
>>>> +
>>>> +#endif /* __INTEL_CMTG_REGS_H__ */
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>> index 4271da219b41..098985ad7ad4 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -62,6 +62,7 @@
>>>> #include "intel_bw.h"
>>>> #include "intel_cdclk.h"
>>>> #include "intel_clock_gating.h"
>>>> +#include "intel_cmtg.h"
>>>> #include "intel_color.h"
>>>> #include "intel_crt.h"
>>>> #include "intel_crtc.h"
>>>> @@ -6828,6 +6829,10 @@ int intel_atomic_check(struct drm_device *dev,
>>>> if (ret)
>>>> goto fail;
>>>>
>>>> + ret = intel_cmtg_atomic_check(state);
>>>> + if (ret)
>>>> + goto fail;
>>>> +
>>>> for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
>>>> if (!intel_crtc_needs_modeset(new_crtc_state))
>>>> continue;
>>>> @@ -7757,6 +7762,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>> intel_modeset_get_crtc_power_domains(new_crtc_state, &put_domains[crtc->pipe]);
>>>> }
>>>>
>>>> + intel_cmtg_disable(state);
>>>> +
>>>> intel_commit_modeset_disables(state);
>>>>
>>>> intel_dp_tunnel_atomic_alloc_bw(state);
>>>> @@ -8582,6 +8589,10 @@ int intel_initial_commit(struct drm_device *dev)
>>>> }
>>>> }
>>>>
>>>> + ret = intel_cmtg_force_disabled(to_intel_atomic_state(state));
>>>> + if (ret)
>>>> + goto out;
>>>> +
>>>
>>>I think the usual way is to do foo_sanitize_state() at
>>>intel_modeset_setup_hw_state().
>>
>> Hm... I see. In this case:
>>
>> - For Xe2_LPD and newer, we can simply disable the CMTG as part of the
>> sanitization;
>> - For pre-Xe2_LPD displays, which would require a modeset when disabling
>> the CMTG, additionally force the CRTC to be disabled.
>>
>> Right?
>
>I'm not sure what you mean by forcing the CRTC to be disabled, but I
>think that's the general idea, yes.
What I thought was that I would have a intel_cmtg_sanitize_state() that
would disable the CMTG.
For platforms pre-LNL (i.e. display pre-Xe2_LPD), disabling the CMTG
requires a modeset. In this case, I was thinking about
intel_cmtg_sanitize_state() returning a mask of pipes that need to be
disabled as part of the sanitization.
I see that intel_sanitize_crtc() disables an active pipe if it does not
have an active connector. So such a mask could be passed to
intel_sanitize_all_crtcs().
Unless there is a better way... Is there a way of flagging that some
pipe needs a modeset during the sanitization step?
>
>> In this case, I guess implementing the whole "global state" protocol for
>> the CMTG wouldn't make much sense if we are not going to handle the
>> disabling as part of the initial commit. We could simply store the state
>> in a "vanilla" struct (and it would be good if such struct lived only
>> during the readout+sanitization time).
>
>I think it all depends on what we'll need if/when we properly implement
>CMTG support. If we're going to need global state for that, and you have
>it written here, might just as well use it instead of throwing it in the
>curb.
Okay. Sounds good.
I guess we could keep the state strucutures, but then, with the CMTG
being disabled during sanitization, we would not need to have those
get/get_new/get_old functions. Probably just the ones for duplicating
and freeing, for consistency.
--
Gustavo Sousa
