| Message ID | 1454514805-10595-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 03, 2016 at 04:53:24PM +0100, Maarten Lankhorst wrote: > This can be derived from the atomic state in pre_plane_update, > which makes it more clear when it's supposed to be called. > > Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++------ > drivers/gpu/drm/i915/intel_drv.h | 1 - > 2 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index de94ef165fbe..c7f2a18ab34e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4807,19 +4807,33 @@ static void intel_post_plane_update(struct intel_crtc *crtc) > memset(atomic, 0, sizeof(*atomic)); > } > > -static void intel_pre_plane_update(struct intel_crtc *crtc) > +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > { > + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc_atomic_commit *atomic = &crtc->atomic; > struct intel_crtc_state *pipe_config = > to_intel_crtc_state(crtc->base.state); > + struct drm_atomic_state *old_state = old_crtc_state->base.state; > + struct drm_plane *primary = crtc->base.primary; > + struct drm_plane_state *old_pri_state = > + drm_atomic_get_existing_plane_state(old_state, primary); > + bool modeset = needs_modeset(&pipe_config->base); > > if (atomic->update_fbc) > intel_fbc_pre_update(crtc); > > - if (atomic->pre_disable_primary) > - intel_pre_disable_primary(&crtc->base); > + if (old_pri_state) { When might we not have and old state for the primary plane? > + struct intel_plane_state *primary_state = > + to_intel_plane_state(primary->state); > + struct intel_plane_state *old_primary_state = > + to_intel_plane_state(old_pri_state); > + > + if (old_primary_state->visible && > + (modeset || !primary_state->visible)) > + intel_pre_disable_primary(&crtc->base); > + } > > if (pipe_config->disable_cxsr) { > crtc->wm.cxsr_allowed = false; > @@ -11883,7 +11897,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > > switch (plane->type) { > case DRM_PLANE_TYPE_PRIMARY: > - intel_crtc->atomic.pre_disable_primary = turn_off; > intel_crtc->atomic.post_enable_primary = turn_on; > intel_crtc->atomic.update_fbc = true; > > @@ -13495,7 +13508,7 @@ static int intel_atomic_commit(struct drm_device *dev, > if (!needs_modeset(crtc->state)) > continue; > > - intel_pre_plane_update(intel_crtc); > + intel_pre_plane_update(to_intel_crtc_state(crtc_state)); > > if (crtc_state->active) { > intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > @@ -13551,7 +13564,7 @@ static int intel_atomic_commit(struct drm_device *dev, > } > > if (!modeset) > - intel_pre_plane_update(intel_crtc); > + intel_pre_plane_update(to_intel_crtc_state(crtc_state)); > > if (crtc->state->active && intel_crtc->atomic.update_fbc) > intel_fbc_enable(intel_crtc); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c9ae2be260d0..167f284ccb16 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -544,7 +544,6 @@ struct intel_mmio_flip { > */ > struct intel_crtc_atomic_commit { > /* Sleepable operations to perform before commit */ > - bool pre_disable_primary; > > /* Sleepable operations to perform after commit */ > unsigned fb_bits; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 03-02-16 om 17:07 schreef Ville Syrjälä: > On Wed, Feb 03, 2016 at 04:53:24PM +0100, Maarten Lankhorst wrote: >> This can be derived from the atomic state in pre_plane_update, >> which makes it more clear when it's supposed to be called. >> >> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++------ >> drivers/gpu/drm/i915/intel_drv.h | 1 - >> 2 files changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index de94ef165fbe..c7f2a18ab34e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4807,19 +4807,33 @@ static void intel_post_plane_update(struct intel_crtc *crtc) >> memset(atomic, 0, sizeof(*atomic)); >> } >> >> -static void intel_pre_plane_update(struct intel_crtc *crtc) >> +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) >> { >> + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc_atomic_commit *atomic = &crtc->atomic; >> struct intel_crtc_state *pipe_config = >> to_intel_crtc_state(crtc->base.state); >> + struct drm_atomic_state *old_state = old_crtc_state->base.state; >> + struct drm_plane *primary = crtc->base.primary; >> + struct drm_plane_state *old_pri_state = >> + drm_atomic_get_existing_plane_state(old_state, primary); >> + bool modeset = needs_modeset(&pipe_config->base); >> >> if (atomic->update_fbc) >> intel_fbc_pre_update(crtc); >> >> - if (atomic->pre_disable_primary) >> - intel_pre_disable_primary(&crtc->base); >> + if (old_pri_state) { > When might we not have and old state for the primary plane? > When updating some state unrelated to the primary plane, for example changing cursor. modeset and update_pipe should always add all planes. ~Maarten
On Wed, Feb 03, 2016 at 06:23:13PM +0100, Maarten Lankhorst wrote: > Op 03-02-16 om 17:07 schreef Ville Syrjälä: > > On Wed, Feb 03, 2016 at 04:53:24PM +0100, Maarten Lankhorst wrote: > >> This can be derived from the atomic state in pre_plane_update, > >> which makes it more clear when it's supposed to be called. > >> > >> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++------ > >> drivers/gpu/drm/i915/intel_drv.h | 1 - > >> 2 files changed, 19 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index de94ef165fbe..c7f2a18ab34e 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -4807,19 +4807,33 @@ static void intel_post_plane_update(struct intel_crtc *crtc) > >> memset(atomic, 0, sizeof(*atomic)); > >> } > >> > >> -static void intel_pre_plane_update(struct intel_crtc *crtc) > >> +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > >> { > >> + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); > >> struct drm_device *dev = crtc->base.dev; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct intel_crtc_atomic_commit *atomic = &crtc->atomic; > >> struct intel_crtc_state *pipe_config = > >> to_intel_crtc_state(crtc->base.state); > >> + struct drm_atomic_state *old_state = old_crtc_state->base.state; > >> + struct drm_plane *primary = crtc->base.primary; > >> + struct drm_plane_state *old_pri_state = > >> + drm_atomic_get_existing_plane_state(old_state, primary); > >> + bool modeset = needs_modeset(&pipe_config->base); > >> > >> if (atomic->update_fbc) > >> intel_fbc_pre_update(crtc); > >> > >> - if (atomic->pre_disable_primary) > >> - intel_pre_disable_primary(&crtc->base); > >> + if (old_pri_state) { > > When might we not have and old state for the primary plane? > > > When updating some state unrelated to the primary plane, for example changing cursor. Hmm. I thought we added all the planes to the state currently... I guess that only happens due to watermark calculation on specific platforms. > > modeset and update_pipe should always add all planes. > > ~Maarten
Op 03-02-16 om 18:39 schreef Ville Syrjälä: > On Wed, Feb 03, 2016 at 06:23:13PM +0100, Maarten Lankhorst wrote: >> Op 03-02-16 om 17:07 schreef Ville Syrjälä: >>> On Wed, Feb 03, 2016 at 04:53:24PM +0100, Maarten Lankhorst wrote: >>>> This can be derived from the atomic state in pre_plane_update, >>>> which makes it more clear when it's supposed to be called. >>>> >>>> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++------ >>>> drivers/gpu/drm/i915/intel_drv.h | 1 - >>>> 2 files changed, 19 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index de94ef165fbe..c7f2a18ab34e 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -4807,19 +4807,33 @@ static void intel_post_plane_update(struct intel_crtc *crtc) >>>> memset(atomic, 0, sizeof(*atomic)); >>>> } >>>> >>>> -static void intel_pre_plane_update(struct intel_crtc *crtc) >>>> +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) >>>> { >>>> + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); >>>> struct drm_device *dev = crtc->base.dev; >>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>> struct intel_crtc_atomic_commit *atomic = &crtc->atomic; >>>> struct intel_crtc_state *pipe_config = >>>> to_intel_crtc_state(crtc->base.state); >>>> + struct drm_atomic_state *old_state = old_crtc_state->base.state; >>>> + struct drm_plane *primary = crtc->base.primary; >>>> + struct drm_plane_state *old_pri_state = >>>> + drm_atomic_get_existing_plane_state(old_state, primary); >>>> + bool modeset = needs_modeset(&pipe_config->base); >>>> >>>> if (atomic->update_fbc) >>>> intel_fbc_pre_update(crtc); >>>> >>>> - if (atomic->pre_disable_primary) >>>> - intel_pre_disable_primary(&crtc->base); >>>> + if (old_pri_state) { >>> When might we not have and old state for the primary plane? >>> >> When updating some state unrelated to the primary plane, for example changing cursor. > Hmm. I thought we added all the planes to the state currently... > I guess that only happens due to watermark calculation on specific > platforms. > Yes we do and that's a bug. I have a patch to reuse the old values for the planes not part of the state, I just didn't send it upstream yet. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index de94ef165fbe..c7f2a18ab34e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4807,19 +4807,33 @@ static void intel_post_plane_update(struct intel_crtc *crtc) memset(atomic, 0, sizeof(*atomic)); } -static void intel_pre_plane_update(struct intel_crtc *crtc) +static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) { + struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc_atomic_commit *atomic = &crtc->atomic; struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); + struct drm_atomic_state *old_state = old_crtc_state->base.state; + struct drm_plane *primary = crtc->base.primary; + struct drm_plane_state *old_pri_state = + drm_atomic_get_existing_plane_state(old_state, primary); + bool modeset = needs_modeset(&pipe_config->base); if (atomic->update_fbc) intel_fbc_pre_update(crtc); - if (atomic->pre_disable_primary) - intel_pre_disable_primary(&crtc->base); + if (old_pri_state) { + struct intel_plane_state *primary_state = + to_intel_plane_state(primary->state); + struct intel_plane_state *old_primary_state = + to_intel_plane_state(old_pri_state); + + if (old_primary_state->visible && + (modeset || !primary_state->visible)) + intel_pre_disable_primary(&crtc->base); + } if (pipe_config->disable_cxsr) { crtc->wm.cxsr_allowed = false; @@ -11883,7 +11897,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, switch (plane->type) { case DRM_PLANE_TYPE_PRIMARY: - intel_crtc->atomic.pre_disable_primary = turn_off; intel_crtc->atomic.post_enable_primary = turn_on; intel_crtc->atomic.update_fbc = true; @@ -13495,7 +13508,7 @@ static int intel_atomic_commit(struct drm_device *dev, if (!needs_modeset(crtc->state)) continue; - intel_pre_plane_update(intel_crtc); + intel_pre_plane_update(to_intel_crtc_state(crtc_state)); if (crtc_state->active) { intel_crtc_disable_planes(crtc, crtc_state->plane_mask); @@ -13551,7 +13564,7 @@ static int intel_atomic_commit(struct drm_device *dev, } if (!modeset) - intel_pre_plane_update(intel_crtc); + intel_pre_plane_update(to_intel_crtc_state(crtc_state)); if (crtc->state->active && intel_crtc->atomic.update_fbc) intel_fbc_enable(intel_crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c9ae2be260d0..167f284ccb16 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -544,7 +544,6 @@ struct intel_mmio_flip { */ struct intel_crtc_atomic_commit { /* Sleepable operations to perform before commit */ - bool pre_disable_primary; /* Sleepable operations to perform after commit */ unsigned fb_bits;