| Message ID | 20191029072229.27092-12-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CI,01/12] drm/i915: Introduce intel_atomic_get_plane_state_after_check(), v2. | expand |
On Tue, Oct 29, 2019 at 08:22:29AM +0100, 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. > > Changes since v1: > - Do not clear hw state on duplication. > > 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 | 23 +++++++++--- > 4 files changed, 57 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index ba7d5421f791..75a5004b234e 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -93,6 +93,10 @@ intel_plane_duplicate_state(struct drm_plane *plane) > intel_state->vma = NULL; > intel_state->flags = 0; > > + /* add reference to fb */ > + if (intel_state->hw.fb) > + drm_framebuffer_get(intel_state->hw.fb); > + > return &intel_state->uapi; > } > > @@ -112,6 +116,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); > } > > @@ -176,15 +182,44 @@ bool intel_plane_calc_min_cdclk(struct intel_atomic_state *state, > return false; > } > > +static void intel_plane_clear_hw_state(struct intel_plane_state *plane_state) > +{ > + if (plane_state->hw.fb) > + drm_framebuffer_put(plane_state->hw.fb); > + > + memset(&plane_state->hw, 0, sizeof(plane_state->hw)); > +} > + > +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, > + const struct intel_plane_state *from_plane_state) > +{ > + intel_plane_clear_hw_state(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); > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h > index e61e9a82aadf..cdb0f97d09f9 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 91a059e5fdcb..ed9504d0336e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -3283,6 +3283,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 e43d5a09550d..344b40687a30 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -523,11 +523,24 @@ struct intel_atomic_state { > }; > > struct intel_plane_state { > - union { > - struct drm_plane_state base; > - 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; u16, though this will easily fit into u8. > + unsigned int rotation; This too can fit into u8. Would be nice to change those types in the core struct too I guess. Otherwise lgtm Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + 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 29-10-2019 om 19:34 schreef Ville Syrjälä: > On Tue, Oct 29, 2019 at 08:22:29AM +0100, 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. >> >> Changes since v1: >> - Do not clear hw state on duplication. >> >> 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 | 23 +++++++++--- >> 4 files changed, 57 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> index ba7d5421f791..75a5004b234e 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >> @@ -93,6 +93,10 @@ intel_plane_duplicate_state(struct drm_plane *plane) >> intel_state->vma = NULL; >> intel_state->flags = 0; >> >> + /* add reference to fb */ >> + if (intel_state->hw.fb) >> + drm_framebuffer_get(intel_state->hw.fb); >> + >> return &intel_state->uapi; >> } >> >> @@ -112,6 +116,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); >> } >> >> @@ -176,15 +182,44 @@ bool intel_plane_calc_min_cdclk(struct intel_atomic_state *state, >> return false; >> } >> >> +static void intel_plane_clear_hw_state(struct intel_plane_state *plane_state) >> +{ >> + if (plane_state->hw.fb) >> + drm_framebuffer_put(plane_state->hw.fb); >> + >> + memset(&plane_state->hw, 0, sizeof(plane_state->hw)); >> +} >> + >> +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, >> + const struct intel_plane_state *from_plane_state) >> +{ >> + intel_plane_clear_hw_state(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); >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h >> index e61e9a82aadf..cdb0f97d09f9 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 91a059e5fdcb..ed9504d0336e 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -3283,6 +3283,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 e43d5a09550d..344b40687a30 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -523,11 +523,24 @@ struct intel_atomic_state { >> }; >> >> struct intel_plane_state { >> - union { >> - struct drm_plane_state base; >> - 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; > u16, though this will easily fit into u8. > >> + unsigned int rotation; > This too can fit into u8. > > Would be nice to change those types in the core struct too I guess. > > Otherwise lgtm > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I deliberately kept the drm core types, so that wouldn't cause problems. I think if we want to change it, we have to change it in core first. Not sure it's worth the time though. :)
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index ba7d5421f791..75a5004b234e 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -93,6 +93,10 @@ intel_plane_duplicate_state(struct drm_plane *plane) intel_state->vma = NULL; intel_state->flags = 0; + /* add reference to fb */ + if (intel_state->hw.fb) + drm_framebuffer_get(intel_state->hw.fb); + return &intel_state->uapi; } @@ -112,6 +116,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); } @@ -176,15 +182,44 @@ bool intel_plane_calc_min_cdclk(struct intel_atomic_state *state, return false; } +static void intel_plane_clear_hw_state(struct intel_plane_state *plane_state) +{ + if (plane_state->hw.fb) + drm_framebuffer_put(plane_state->hw.fb); + + memset(&plane_state->hw, 0, sizeof(plane_state->hw)); +} + +void intel_plane_copy_uapi_to_hw_state(struct intel_plane_state *plane_state, + const struct intel_plane_state *from_plane_state) +{ + intel_plane_clear_hw_state(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); diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h index e61e9a82aadf..cdb0f97d09f9 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 91a059e5fdcb..ed9504d0336e 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -3283,6 +3283,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 e43d5a09550d..344b40687a30 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -523,11 +523,24 @@ struct intel_atomic_state { }; struct intel_plane_state { - union { - struct drm_plane_state base; - 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. Changes since v1: - Do not clear hw state on duplication. 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 | 23 +++++++++--- 4 files changed, 57 insertions(+), 6 deletions(-)