| Message ID | 1476885339-2265-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > if (nonblock) > queue_work(system_unbound_wq, &state->commit_work); > else > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; > + } The placement after the _get() looks a bit weird. The other parts of the state were flipped over before the _get(), so putting this next to the other would make it look less magicy. > + > if (nonblock) > queue_work(system_unbound_wq, &state->commit_work); > else > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 21-10-16 om 16:21 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; >> + } > The placement after the _get() looks a bit weird. The other parts of the > state were flipped over before the _get(), so putting this next to the > other would make it look less magicy. > That's fine, this patch was rebased after thje drm_atomic_state_get was added, which explains the placement. ~Maarten
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. 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. ~Maarten
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?
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
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; + } + if (nonblock) queue_work(system_unbound_wq, &state->commit_work); else
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(-)