Re: [RESEND PATCH 4/6] drm/i915: Update atomic modeset state synchronously

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 24-10-16 om 12:19 schreef Ville Syrjälä:
> On Mon, Oct 24, 2016 at 12:07:30PM +0200, Maarten Lankhorst wrote:
>> Op 21-10-16 om 16:08 schreef Ville Syrjälä:
>>> On Wed, Oct 19, 2016 at 03:55:37PM +0200, Maarten Lankhorst wrote:
>>>> All of this state should be updated as soon as possible. It shouldn't be
>>>> done later because then future updates may not depend on it.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++-------
>>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 69b9e91f071e..ba7f7be3aa4f 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -14341,14 +14341,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>>>  
>>>>  	drm_atomic_helper_wait_for_dependencies(state);
>>>>  
>>>> -	if (intel_state->modeset) {
>>>> -		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>>>> -		       sizeof(intel_state->min_pixclk));
>>>> -		dev_priv->active_crtcs = intel_state->active_crtcs;
>>>> -		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>>>> -
>>>> +	if (intel_state->modeset)
>>>>  		intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
>>>> -	}
>>>>  
>>>>  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>> @@ -14558,6 +14552,13 @@ static int intel_atomic_commit(struct drm_device *dev,
>>>>  	intel_atomic_track_fbs(state);
>>>>  
>>>>  	drm_atomic_state_get(state);
>>>> +	if (intel_state->modeset) {
>>>> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>>>> +		       sizeof(intel_state->min_pixclk));
>>>> +		dev_priv->active_crtcs = intel_state->active_crtcs;
>>>> +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
>>>> +	}
>>>> +
>>> I'm not very happy about this whole intel_atomic_state <-> dev_priv
>>> mess. I think what might be nicer is to have an intel_atomic_device_state
>>> or something, which would be part of the top level atomic state just
>>> like the other states, and we would just a pointer to the current
>>> device state under dev_priv I suppose. This way the top level atomic
>>> state would be more like an atomic transaction thing.
>>>
>> Neither, but I don't see a way to separate it cleanly. The updated members are too random to be put together in a struct.
> What do you mean random? Just not related to each other. That doesn't
> prohibit from collecting them up in a struct. They're already there
> in intel_atomic_state after all.
>
>> And some are used read-only when !modeset, and others are not meant to be used at all in that case. Might even depend
>> on the platform.
> How is one supposed to tell when they're to be used or not?
Outside of modeset I think only intel_atomic_state->cdclk is valid. But only used by skl watermark sanitization. It uses a trick
in which intel_state->active_crtcs is valid too. I think that requirement can be removed though with some small changes. It
looks wrong anyway..

Some more looking shows that the wrong cdclk is used for scaler calculations with !modeset, only matters in the dpms off case.
But that needs fixing too..

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx