| Message ID | 20191004113514.17064-13-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable bigjoiner support, second approach. | expand |
On Fri, Oct 04, 2019 at 01:35:02PM +0200, Maarten Lankhorst wrote: > Splitting plane state is easier than splitting crtc_state, > before plane check we copy the drm properties to hw so we can > do the same in bigjoiner later on. > > We copy the state after we did all the modeset handling, but fortunately > i915 seems to be split correctly and nothing during modeset looks > at plane_state. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- > .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + > drivers/gpu/drm/i915/display/intel_display.c | 1 + > .../drm/i915/display/intel_display_types.h | 22 +++++++++-- > 4 files changed, 57 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 01937896d69c..cc154cfa3381 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) > intel_state->vma = NULL; > intel_state->flags = 0; > > + /* will be set in intel_plane_atomic_check_with_state() */ > + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); That seems wrong for the case where we add the plane after the plane check has already been done. I think we should maintain the current state unless the plane check will overwrite it. > + > return &intel_state->uapi; > } > > @@ -112,6 +115,8 @@ intel_plane_destroy_state(struct drm_plane *plane, > WARN_ON(plane_state->vma); > > __drm_atomic_helper_plane_destroy_state(&plane_state->uapi); > + if (plane_state->hw.fb) > + drm_framebuffer_put(plane_state->hw.fb); > kfree(plane_state); > } > > @@ -138,15 +143,34 @@ unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, > return cpp * crtc_state->pixel_rate; > } > > +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, > + const struct intel_plane_state *from_plane_state) > +{ > + plane_state->hw.crtc = from_plane_state->uapi.crtc; > + plane_state->hw.fb = from_plane_state->uapi.fb; > + if (plane_state->hw.fb) > + drm_framebuffer_get(plane_state->hw.fb); > + > + plane_state->hw.alpha = from_plane_state->uapi.alpha; > + plane_state->hw.pixel_blend_mode = > + from_plane_state->uapi.pixel_blend_mode; > + plane_state->hw.rotation = from_plane_state->uapi.rotation; > + plane_state->hw.color_encoding = from_plane_state->uapi.color_encoding; > + plane_state->hw.color_range = from_plane_state->uapi.color_range; > +} > + > int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state, > struct intel_crtc_state *new_crtc_state, > const struct intel_plane_state *old_plane_state, > struct intel_plane_state *new_plane_state) > { > struct intel_plane *plane = to_intel_plane(new_plane_state->uapi.plane); > - const struct drm_framebuffer *fb = new_plane_state->hw.fb; > + const struct drm_framebuffer *fb; > int ret; > > + intel_plane_copy_uapi_to_hw_state(new_plane_state, new_plane_state); > + fb = new_plane_state->hw.fb; > + > new_crtc_state->active_planes &= ~BIT(plane->id); > new_crtc_state->nv12_planes &= ~BIT(plane->id); > new_crtc_state->c8_planes &= ~BIT(plane->id); > @@ -373,6 +397,7 @@ intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, > { > struct intel_plane_state *plane_state = > intel_atomic_get_new_plane_state(state, plane); > + const struct intel_plane_state *old_plane_state; > > if (plane_state) > return plane_state; > @@ -381,6 +406,16 @@ intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, > if (IS_ERR(plane_state)) > return plane_state; > > + old_plane_state = intel_atomic_get_old_plane_state(state, plane); > + > + /* > + * copy HW parameters since they're zero'd on duplication, to prevent > + * accidentally using stale state. > + */ > + plane_state->hw = old_plane_state->hw; > + if (plane_state->hw.fb) > + drm_framebuffer_get(plane_state->hw.fb); > + > new_crtc_state->update_planes |= BIT(plane->id); > return plane_state; > } > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > index cb7ef4f9eafd..fd4b96013454 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > @@ -20,6 +20,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; > > unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state); > +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, > + const struct intel_plane_state *from_plane_state); > void intel_update_plane(struct intel_plane *plane, > const struct intel_crtc_state *crtc_state, > const struct intel_plane_state *plane_state); > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 0ee542840c70..ffde4b8b8be9 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -3267,6 +3267,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > > plane_state->fb = fb; > plane_state->crtc = &intel_crtc->base; > + intel_plane_copy_uapi_to_hw_state(intel_state, intel_state); > > atomic_or(to_intel_plane(primary)->frontbuffer_bit, > &to_intel_frontbuffer(fb)->bits); > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index c6c49aa8c172..67644818efd2 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -515,10 +515,24 @@ struct intel_atomic_state { > }; > > struct intel_plane_state { > - union { > - struct drm_plane_state uapi; > - struct drm_plane_state hw; > - }; > + struct drm_plane_state uapi; > + > + /* > + * actual hardware state, the state we program to the hardware. > + * The following members are used to verify the hardware state: > + * During initial hw readout, they need to be copied from uapi. > + */ > + struct { > + struct drm_crtc *crtc; > + struct drm_framebuffer *fb; > + > + u16 alpha; > + uint16_t pixel_blend_mode; > + unsigned int rotation; > + enum drm_color_encoding color_encoding; > + enum drm_color_range color_range; > + } hw; > + > struct i915_ggtt_view view; > struct i915_vma *vma; > unsigned long flags; > -- > 2.23.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 08-10-2019 om 19:42 schreef Ville Syrjälä: > On Fri, Oct 04, 2019 at 01:35:02PM +0200, Maarten Lankhorst wrote: >> Splitting plane state is easier than splitting crtc_state, >> before plane check we copy the drm properties to hw so we can >> do the same in bigjoiner later on. >> >> We copy the state after we did all the modeset handling, but fortunately >> i915 seems to be split correctly and nothing during modeset looks >> at plane_state. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- >> .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + >> drivers/gpu/drm/i915/display/intel_display.c | 1 + >> .../drm/i915/display/intel_display_types.h | 22 +++++++++-- >> 4 files changed, 57 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> index 01937896d69c..cc154cfa3381 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) >> intel_state->vma = NULL; >> intel_state->flags = 0; >> >> + /* will be set in intel_plane_atomic_check_with_state() */ >> + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); > That seems wrong for the case where we add the plane after the > plane check has already been done. > > I think we should maintain the current state unless the plane check > will overwrite it. Have you seen intel_atomic_get_plane_state_after_check()? >> + >> return &intel_state->uapi; >> } >> >> @@ -112,6 +115,8 @@ intel_plane_destroy_state(struct drm_plane *plane, >> WARN_ON(plane_state->vma); >> >> __drm_atomic_helper_plane_destroy_state(&plane_state->uapi); >> + if (plane_state->hw.fb) >> + drm_framebuffer_put(plane_state->hw.fb); >> kfree(plane_state); >> } >> >> @@ -138,15 +143,34 @@ unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, >> return cpp * crtc_state->pixel_rate; >> } >> >> +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, >> + const struct intel_plane_state *from_plane_state) >> +{ >> + plane_state->hw.crtc = from_plane_state->uapi.crtc; >> + plane_state->hw.fb = from_plane_state->uapi.fb; >> + if (plane_state->hw.fb) >> + drm_framebuffer_get(plane_state->hw.fb); >> + >> + plane_state->hw.alpha = from_plane_state->uapi.alpha; >> + plane_state->hw.pixel_blend_mode = >> + from_plane_state->uapi.pixel_blend_mode; >> + plane_state->hw.rotation = from_plane_state->uapi.rotation; >> + plane_state->hw.color_encoding = from_plane_state->uapi.color_encoding; >> + plane_state->hw.color_range = from_plane_state->uapi.color_range; >> +} >> + >> int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state, >> struct intel_crtc_state *new_crtc_state, >> const struct intel_plane_state *old_plane_state, >> struct intel_plane_state *new_plane_state) >> { >> struct intel_plane *plane = to_intel_plane(new_plane_state->uapi.plane); >> - const struct drm_framebuffer *fb = new_plane_state->hw.fb; >> + const struct drm_framebuffer *fb; >> int ret; >> >> + intel_plane_copy_uapi_to_hw_state(new_plane_state, new_plane_state); >> + fb = new_plane_state->hw.fb; >> + >> new_crtc_state->active_planes &= ~BIT(plane->id); >> new_crtc_state->nv12_planes &= ~BIT(plane->id); >> new_crtc_state->c8_planes &= ~BIT(plane->id); >> @@ -373,6 +397,7 @@ intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, >> { >> struct intel_plane_state *plane_state = >> intel_atomic_get_new_plane_state(state, plane); >> + const struct intel_plane_state *old_plane_state; >> >> if (plane_state) >> return plane_state; >> @@ -381,6 +406,16 @@ intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, >> if (IS_ERR(plane_state)) >> return plane_state; >> >> + old_plane_state = intel_atomic_get_old_plane_state(state, plane); >> + >> + /* >> + * copy HW parameters since they're zero'd on duplication, to prevent >> + * accidentally using stale state. >> + */ >> + plane_state->hw = old_plane_state->hw; >> + if (plane_state->hw.fb) >> + drm_framebuffer_get(plane_state->hw.fb); >> + >> new_crtc_state->update_planes |= BIT(plane->id); >> return plane_state; >> } >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> index cb7ef4f9eafd..fd4b96013454 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> @@ -20,6 +20,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; >> >> unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, >> const struct intel_plane_state *plane_state); >> +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, >> + const struct intel_plane_state *from_plane_state); >> void intel_update_plane(struct intel_plane *plane, >> const struct intel_crtc_state *crtc_state, >> const struct intel_plane_state *plane_state); >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index 0ee542840c70..ffde4b8b8be9 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -3267,6 +3267,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, >> >> plane_state->fb = fb; >> plane_state->crtc = &intel_crtc->base; >> + intel_plane_copy_uapi_to_hw_state(intel_state, intel_state); >> >> atomic_or(to_intel_plane(primary)->frontbuffer_bit, >> &to_intel_frontbuffer(fb)->bits); >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h >> index c6c49aa8c172..67644818efd2 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -515,10 +515,24 @@ struct intel_atomic_state { >> }; >> >> struct intel_plane_state { >> - union { >> - struct drm_plane_state uapi; >> - struct drm_plane_state hw; >> - }; >> + struct drm_plane_state uapi; >> + >> + /* >> + * actual hardware state, the state we program to the hardware. >> + * The following members are used to verify the hardware state: >> + * During initial hw readout, they need to be copied from uapi. >> + */ >> + struct { >> + struct drm_crtc *crtc; >> + struct drm_framebuffer *fb; >> + >> + u16 alpha; >> + uint16_t pixel_blend_mode; >> + unsigned int rotation; >> + enum drm_color_encoding color_encoding; >> + enum drm_color_range color_range; >> + } hw; >> + >> struct i915_ggtt_view view; >> struct i915_vma *vma; >> unsigned long flags; >> -- >> 2.23.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 09, 2019 at 02:13:55PM +0200, Maarten Lankhorst wrote: > Op 08-10-2019 om 19:42 schreef Ville Syrjälä: > > On Fri, Oct 04, 2019 at 01:35:02PM +0200, Maarten Lankhorst wrote: > >> Splitting plane state is easier than splitting crtc_state, > >> before plane check we copy the drm properties to hw so we can > >> do the same in bigjoiner later on. > >> > >> We copy the state after we did all the modeset handling, but fortunately > >> i915 seems to be split correctly and nothing during modeset looks > >> at plane_state. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- > >> .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + > >> drivers/gpu/drm/i915/display/intel_display.c | 1 + > >> .../drm/i915/display/intel_display_types.h | 22 +++++++++-- > >> 4 files changed, 57 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> index 01937896d69c..cc154cfa3381 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) > >> intel_state->vma = NULL; > >> intel_state->flags = 0; > >> > >> + /* will be set in intel_plane_atomic_check_with_state() */ > >> + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); > > That seems wrong for the case where we add the plane after the > > plane check has already been done. > > > > I think we should maintain the current state unless the plane check > > will overwrite it. > Have you seen > > intel_atomic_get_plane_state_after_check()? Yes, but I don't like it.
Op 09-10-2019 om 14:23 schreef Ville Syrjälä: > On Wed, Oct 09, 2019 at 02:13:55PM +0200, Maarten Lankhorst wrote: >> Op 08-10-2019 om 19:42 schreef Ville Syrjälä: >>> On Fri, Oct 04, 2019 at 01:35:02PM +0200, Maarten Lankhorst wrote: >>>> Splitting plane state is easier than splitting crtc_state, >>>> before plane check we copy the drm properties to hw so we can >>>> do the same in bigjoiner later on. >>>> >>>> We copy the state after we did all the modeset handling, but fortunately >>>> i915 seems to be split correctly and nothing during modeset looks >>>> at plane_state. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- >>>> .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + >>>> drivers/gpu/drm/i915/display/intel_display.c | 1 + >>>> .../drm/i915/display/intel_display_types.h | 22 +++++++++-- >>>> 4 files changed, 57 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> index 01937896d69c..cc154cfa3381 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>> @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) >>>> intel_state->vma = NULL; >>>> intel_state->flags = 0; >>>> >>>> + /* will be set in intel_plane_atomic_check_with_state() */ >>>> + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); >>> That seems wrong for the case where we add the plane after the >>> plane check has already been done. >>> >>> I think we should maintain the current state unless the plane check >>> will overwrite it. >> Have you seen >> >> intel_atomic_get_plane_state_after_check()? > Yes, but I don't like it. This is however required because of the split. We should not look at the old plane_state contents before atomic_check and to prevent that I think it's better to zero out plane_state->hw, in which case bugs are at least consistent. :) ~Maarten
On Wed, Oct 09, 2019 at 02:31:58PM +0200, Maarten Lankhorst wrote: > Op 09-10-2019 om 14:23 schreef Ville Syrjälä: > > On Wed, Oct 09, 2019 at 02:13:55PM +0200, Maarten Lankhorst wrote: > >> Op 08-10-2019 om 19:42 schreef Ville Syrjälä: > >>> On Fri, Oct 04, 2019 at 01:35:02PM +0200, Maarten Lankhorst wrote: > >>>> Splitting plane state is easier than splitting crtc_state, > >>>> before plane check we copy the drm properties to hw so we can > >>>> do the same in bigjoiner later on. > >>>> > >>>> We copy the state after we did all the modeset handling, but fortunately > >>>> i915 seems to be split correctly and nothing during modeset looks > >>>> at plane_state. > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- > >>>> .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + > >>>> drivers/gpu/drm/i915/display/intel_display.c | 1 + > >>>> .../drm/i915/display/intel_display_types.h | 22 +++++++++-- > >>>> 4 files changed, 57 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> index 01937896d69c..cc154cfa3381 100644 > >>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >>>> @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) > >>>> intel_state->vma = NULL; > >>>> intel_state->flags = 0; > >>>> > >>>> + /* will be set in intel_plane_atomic_check_with_state() */ > >>>> + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); > >>> That seems wrong for the case where we add the plane after the > >>> plane check has already been done. > >>> > >>> I think we should maintain the current state unless the plane check > >>> will overwrite it. > >> Have you seen > >> > >> intel_atomic_get_plane_state_after_check()? > > Yes, but I don't like it. > > This is however required because of the split. We should not look at the old plane_state contents before atomic_check and to prevent that I think it's better to zero out plane_state->hw, > > in which case bugs are at least consistent. :) I don't like leaking this special case all over. IMO it's not really any different to the current rule that you can't look at any of the derived state before plane check(). We're just extending the concept of the derived state a bit.
Op 09-10-2019 om 14:41 schreef Ville Syrjälä: > On Wed, Oct 09, 2019 at 02:31:58PM +0200, Maarten Lankhorst wrote: >> Op 09-10-2019 om 14:23 schreef Ville Syrjälä: >>> On Wed, Oct 09, 2019 at 02:13:55PM +0200, Maarten Lankhorst wrote: >>>> Op 08-10-2019 om 19:42 schreef Ville Syrjälä: >>>>> On Fri, Oct 04, 2019 at 01:35:02PM +0200, Maarten Lankhorst wrote: >>>>>> Splitting plane state is easier than splitting crtc_state, >>>>>> before plane check we copy the drm properties to hw so we can >>>>>> do the same in bigjoiner later on. >>>>>> >>>>>> We copy the state after we did all the modeset handling, but fortunately >>>>>> i915 seems to be split correctly and nothing during modeset looks >>>>>> at plane_state. >>>>>> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>> --- >>>>>> .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- >>>>>> .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + >>>>>> drivers/gpu/drm/i915/display/intel_display.c | 1 + >>>>>> .../drm/i915/display/intel_display_types.h | 22 +++++++++-- >>>>>> 4 files changed, 57 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> index 01937896d69c..cc154cfa3381 100644 >>>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>>> @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) >>>>>> intel_state->vma = NULL; >>>>>> intel_state->flags = 0; >>>>>> >>>>>> + /* will be set in intel_plane_atomic_check_with_state() */ >>>>>> + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); >>>>> That seems wrong for the case where we add the plane after the >>>>> plane check has already been done. >>>>> >>>>> I think we should maintain the current state unless the plane check >>>>> will overwrite it. >>>> Have you seen >>>> >>>> intel_atomic_get_plane_state_after_check()? >>> Yes, but I don't like it. >> This is however required because of the split. We should not look at the old plane_state contents before atomic_check and to prevent that I think it's better to zero out plane_state->hw, >> >> in which case bugs are at least consistent. :) > I don't like leaking this special case all over. IMO it's not really any > different to the current rule that you can't look at any of the derived > state before plane check(). We're just extending the concept of the > derived state a bit. > I'm ok with that..
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 01937896d69c..cc154cfa3381 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -93,6 +93,9 @@ intel_plane_duplicate_state(struct drm_plane *plane) intel_state->vma = NULL; intel_state->flags = 0; + /* will be set in intel_plane_atomic_check_with_state() */ + memset(&intel_state->hw, 0, sizeof(intel_state->hw)); + return &intel_state->uapi; } @@ -112,6 +115,8 @@ intel_plane_destroy_state(struct drm_plane *plane, WARN_ON(plane_state->vma); __drm_atomic_helper_plane_destroy_state(&plane_state->uapi); + if (plane_state->hw.fb) + drm_framebuffer_put(plane_state->hw.fb); kfree(plane_state); } @@ -138,15 +143,34 @@ unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, return cpp * crtc_state->pixel_rate; } +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, + const struct intel_plane_state *from_plane_state) +{ + plane_state->hw.crtc = from_plane_state->uapi.crtc; + plane_state->hw.fb = from_plane_state->uapi.fb; + if (plane_state->hw.fb) + drm_framebuffer_get(plane_state->hw.fb); + + plane_state->hw.alpha = from_plane_state->uapi.alpha; + plane_state->hw.pixel_blend_mode = + from_plane_state->uapi.pixel_blend_mode; + plane_state->hw.rotation = from_plane_state->uapi.rotation; + plane_state->hw.color_encoding = from_plane_state->uapi.color_encoding; + plane_state->hw.color_range = from_plane_state->uapi.color_range; +} + int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state, struct intel_crtc_state *new_crtc_state, const struct intel_plane_state *old_plane_state, struct intel_plane_state *new_plane_state) { struct intel_plane *plane = to_intel_plane(new_plane_state->uapi.plane); - const struct drm_framebuffer *fb = new_plane_state->hw.fb; + const struct drm_framebuffer *fb; int ret; + intel_plane_copy_uapi_to_hw_state(new_plane_state, new_plane_state); + fb = new_plane_state->hw.fb; + new_crtc_state->active_planes &= ~BIT(plane->id); new_crtc_state->nv12_planes &= ~BIT(plane->id); new_crtc_state->c8_planes &= ~BIT(plane->id); @@ -373,6 +397,7 @@ intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, { struct intel_plane_state *plane_state = intel_atomic_get_new_plane_state(state, plane); + const struct intel_plane_state *old_plane_state; if (plane_state) return plane_state; @@ -381,6 +406,16 @@ intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, if (IS_ERR(plane_state)) return plane_state; + old_plane_state = intel_atomic_get_old_plane_state(state, plane); + + /* + * copy HW parameters since they're zero'd on duplication, to prevent + * accidentally using stale state. + */ + plane_state->hw = old_plane_state->hw; + if (plane_state->hw.fb) + drm_framebuffer_get(plane_state->hw.fb); + new_crtc_state->update_planes |= BIT(plane->id); return plane_state; } diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h index cb7ef4f9eafd..fd4b96013454 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h @@ -20,6 +20,8 @@ extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state); +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, + const struct intel_plane_state *from_plane_state); void intel_update_plane(struct intel_plane *plane, const struct intel_crtc_state *crtc_state, const struct intel_plane_state *plane_state); diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 0ee542840c70..ffde4b8b8be9 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -3267,6 +3267,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, plane_state->fb = fb; plane_state->crtc = &intel_crtc->base; + intel_plane_copy_uapi_to_hw_state(intel_state, intel_state); atomic_or(to_intel_plane(primary)->frontbuffer_bit, &to_intel_frontbuffer(fb)->bits); diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index c6c49aa8c172..67644818efd2 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -515,10 +515,24 @@ struct intel_atomic_state { }; struct intel_plane_state { - union { - struct drm_plane_state uapi; - struct drm_plane_state hw; - }; + struct drm_plane_state uapi; + + /* + * actual hardware state, the state we program to the hardware. + * The following members are used to verify the hardware state: + * During initial hw readout, they need to be copied from uapi. + */ + struct { + struct drm_crtc *crtc; + struct drm_framebuffer *fb; + + u16 alpha; + uint16_t pixel_blend_mode; + unsigned int rotation; + enum drm_color_encoding color_encoding; + enum drm_color_range color_range; + } hw; + struct i915_ggtt_view view; struct i915_vma *vma; unsigned long flags;
Splitting plane state is easier than splitting crtc_state, before plane check we copy the drm properties to hw so we can do the same in bigjoiner later on. We copy the state after we did all the modeset handling, but fortunately i915 seems to be split correctly and nothing during modeset looks at plane_state. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- .../gpu/drm/i915/display/intel_atomic_plane.c | 37 ++++++++++++++++++- .../gpu/drm/i915/display/intel_atomic_plane.h | 2 + drivers/gpu/drm/i915/display/intel_display.c | 1 + .../drm/i915/display/intel_display_types.h | 22 +++++++++-- 4 files changed, 57 insertions(+), 5 deletions(-)