| Message ID | 1441894085-25662-7-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote: > This should allow not running plane commit when the crtc is off. > While the atomic helpers update those, crtc->x/y is only updated > during modesets, and primary plane is updated after this function > returns. > > Unfortunately non-atomic watermarks and fbc still depend on this > state inside i915, so it has to be kept in sync. > > 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 87c5eba08454..b809ee2a8678 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > crtc->hwmode = crtc->state->adjusted_mode; > else > crtc->hwmode.crtc_clock = 0; > + > + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { > + struct drm_plane_state *plane_state = crtc->primary->state; > + > + crtc->primary->fb = plane_state->fb; > + crtc->x = plane_state->src_x >> 16; > + crtc->y = plane_state->src_y >> 16; > + } drm_atomic_helper_update_legacy_modeset_state should do that for us, not? Atm we have an if (any_ms) check for it, but that seems to just be a bug really. -Daniel > } > } > > @@ -13434,15 +13442,8 @@ intel_commit_primary_plane(struct drm_plane *plane, > struct drm_framebuffer *fb = state->base.fb; > struct drm_device *dev = plane->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc *intel_crtc; > - struct drm_rect *src = &state->src; > > crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > - plane->fb = fb; > - crtc->x = src->x1 >> 16; > - crtc->y = src->y1 >> 16; > > if (!crtc->state->active) > return; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 10-09-15 om 17:41 schreef Daniel Vetter: > On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote: >> This should allow not running plane commit when the crtc is off. >> While the atomic helpers update those, crtc->x/y is only updated >> during modesets, and primary plane is updated after this function >> returns. >> >> Unfortunately non-atomic watermarks and fbc still depend on this >> state inside i915, so it has to be kept in sync. >> >> 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 87c5eba08454..b809ee2a8678 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) >> crtc->hwmode = crtc->state->adjusted_mode; >> else >> crtc->hwmode.crtc_clock = 0; >> + >> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { >> + struct drm_plane_state *plane_state = crtc->primary->state; >> + >> + crtc->primary->fb = plane_state->fb; >> + crtc->x = plane_state->src_x >> 16; >> + crtc->y = plane_state->src_y >> 16; >> + } > drm_atomic_helper_update_legacy_modeset_state should do that for us, not? > Atm we have an if (any_ms) check for it, but that seems to just be a bug > really. That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state. There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc. ~Maarten
On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote: > Op 10-09-15 om 17:41 schreef Daniel Vetter: > > On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote: > >> This should allow not running plane commit when the crtc is off. > >> While the atomic helpers update those, crtc->x/y is only updated > >> during modesets, and primary plane is updated after this function > >> returns. > >> > >> Unfortunately non-atomic watermarks and fbc still depend on this > >> state inside i915, so it has to be kept in sync. > >> > >> 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 87c5eba08454..b809ee2a8678 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > >> crtc->hwmode = crtc->state->adjusted_mode; > >> else > >> crtc->hwmode.crtc_clock = 0; > >> + > >> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { > >> + struct drm_plane_state *plane_state = crtc->primary->state; > >> + > >> + crtc->primary->fb = plane_state->fb; > >> + crtc->x = plane_state->src_x >> 16; > >> + crtc->y = plane_state->src_y >> 16; > >> + } > > drm_atomic_helper_update_legacy_modeset_state should do that for us, not? > > Atm we have an if (any_ms) check for it, but that seems to just be a bug > > really. > That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit > after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state. > > There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc. Hm, I tried looking really hard but didn't find anything that cared about plane->fb. Can you please point me at them? -Daniel
Op 14-09-15 om 15:13 schreef Daniel Vetter: > On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote: >> Op 10-09-15 om 17:41 schreef Daniel Vetter: >>> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote: >>>> This should allow not running plane commit when the crtc is off. >>>> While the atomic helpers update those, crtc->x/y is only updated >>>> during modesets, and primary plane is updated after this function >>>> returns. >>>> >>>> Unfortunately non-atomic watermarks and fbc still depend on this >>>> state inside i915, so it has to be kept in sync. >>>> >>>> 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 87c5eba08454..b809ee2a8678 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) >>>> crtc->hwmode = crtc->state->adjusted_mode; >>>> else >>>> crtc->hwmode.crtc_clock = 0; >>>> + >>>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { >>>> + struct drm_plane_state *plane_state = crtc->primary->state; >>>> + >>>> + crtc->primary->fb = plane_state->fb; >>>> + crtc->x = plane_state->src_x >> 16; >>>> + crtc->y = plane_state->src_y >> 16; >>>> + } >>> drm_atomic_helper_update_legacy_modeset_state should do that for us, not? >>> Atm we have an if (any_ms) check for it, but that seems to just be a bug >>> really. >> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit >> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state. >> >> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc. > Hm, I tried looking really hard but didn't find anything that cared about > plane->fb. Can you please point me at them? > Look for primary->fb. :-) Also crtc->y is used in __intel_fbc_update. ~Maarten
On Mon, Sep 14, 2015 at 03:16:20PM +0200, Maarten Lankhorst wrote: > Op 14-09-15 om 15:13 schreef Daniel Vetter: > > On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote: > >> Op 10-09-15 om 17:41 schreef Daniel Vetter: > >>> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote: > >>>> This should allow not running plane commit when the crtc is off. > >>>> While the atomic helpers update those, crtc->x/y is only updated > >>>> during modesets, and primary plane is updated after this function > >>>> returns. > >>>> > >>>> Unfortunately non-atomic watermarks and fbc still depend on this > >>>> state inside i915, so it has to be kept in sync. > >>>> > >>>> 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 87c5eba08454..b809ee2a8678 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_display.c > >>>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) > >>>> crtc->hwmode = crtc->state->adjusted_mode; > >>>> else > >>>> crtc->hwmode.crtc_clock = 0; > >>>> + > >>>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { > >>>> + struct drm_plane_state *plane_state = crtc->primary->state; > >>>> + > >>>> + crtc->primary->fb = plane_state->fb; > >>>> + crtc->x = plane_state->src_x >> 16; > >>>> + crtc->y = plane_state->src_y >> 16; > >>>> + } > >>> drm_atomic_helper_update_legacy_modeset_state should do that for us, not? > >>> Atm we have an if (any_ms) check for it, but that seems to just be a bug > >>> really. > >> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit > >> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state. > >> > >> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc. > > Hm, I tried looking really hard but didn't find anything that cared about > > plane->fb. Can you please point me at them? > > > Look for primary->fb. :-) Also crtc->y is used in __intel_fbc_update. Well I did hunt for that but only found usage in crtc_disable functions (where we need the old value anyway) and flip code (which updates it itself). I did indeed somehow miss the fbc code. Oh well. Comment might be good for that. -Daniel
Op 14-09-15 om 16:26 schreef Daniel Vetter: > On Mon, Sep 14, 2015 at 03:16:20PM +0200, Maarten Lankhorst wrote: >> Op 14-09-15 om 15:13 schreef Daniel Vetter: >>> On Mon, Sep 14, 2015 at 11:52:26AM +0200, Maarten Lankhorst wrote: >>>> Op 10-09-15 om 17:41 schreef Daniel Vetter: >>>>> On Thu, Sep 10, 2015 at 04:08:01PM +0200, Maarten Lankhorst wrote: >>>>>> This should allow not running plane commit when the crtc is off. >>>>>> While the atomic helpers update those, crtc->x/y is only updated >>>>>> during modesets, and primary plane is updated after this function >>>>>> returns. >>>>>> >>>>>> Unfortunately non-atomic watermarks and fbc still depend on this >>>>>> state inside i915, so it has to be kept in sync. >>>>>> >>>>>> 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 87c5eba08454..b809ee2a8678 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>> @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) >>>>>> crtc->hwmode = crtc->state->adjusted_mode; >>>>>> else >>>>>> crtc->hwmode.crtc_clock = 0; >>>>>> + >>>>>> + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { >>>>>> + struct drm_plane_state *plane_state = crtc->primary->state; >>>>>> + >>>>>> + crtc->primary->fb = plane_state->fb; >>>>>> + crtc->x = plane_state->src_x >> 16; >>>>>> + crtc->y = plane_state->src_y >> 16; >>>>>> + } >>>>> drm_atomic_helper_update_legacy_modeset_state should do that for us, not? >>>>> Atm we have an if (any_ms) check for it, but that seems to just be a bug >>>>> really. >>>> That function doesn't update primary->fb though. This is fixed up by the caller of drm_atomic_commit >>>> after the call succeeds. We need it sooner because update_fbc unfortunately still looks at the legacy state. >>>> >>>> There are a few other users of the legacy state left in i915, but they should be more easy to convert than fbc. >>> Hm, I tried looking really hard but didn't find anything that cared about >>> plane->fb. Can you please point me at them? >>> >> Look for primary->fb. :-) Also crtc->y is used in __intel_fbc_update. > Well I did hunt for that but only found usage in crtc_disable functions > (where we need the old value anyway) and flip code (which updates it > itself). I did indeed somehow miss the fbc code. Oh well. Comment might be > good for that. > -Daniel Ok, I will resend the remaining patches from this series and this one with the comment added.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 87c5eba08454..b809ee2a8678 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12198,6 +12198,14 @@ intel_modeset_update_crtc_state(struct drm_atomic_state *state) crtc->hwmode = crtc->state->adjusted_mode; else crtc->hwmode.crtc_clock = 0; + + if (drm_atomic_get_existing_plane_state(state, crtc->primary)) { + struct drm_plane_state *plane_state = crtc->primary->state; + + crtc->primary->fb = plane_state->fb; + crtc->x = plane_state->src_x >> 16; + crtc->y = plane_state->src_y >> 16; + } } } @@ -13434,15 +13442,8 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_framebuffer *fb = state->base.fb; struct drm_device *dev = plane->dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc; - struct drm_rect *src = &state->src; crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); - - plane->fb = fb; - crtc->x = src->x1 >> 16; - crtc->y = src->y1 >> 16; if (!crtc->state->active) return;
This should allow not running plane commit when the crtc is off. While the atomic helpers update those, crtc->x/y is only updated during modesets, and primary plane is updated after this function returns. Unfortunately non-atomic watermarks and fbc still depend on this state inside i915, so it has to be kept in sync. 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(-)