On Thu, Oct 17, 2019 at 03:20:59PM +0200, Maarten Lankhorst wrote:
> Now that we separated everything into uapi and hw, it's
> time to make the split definitive. Remove the union and
> make a copy of the hw state on modeset and fastset.
>
> Color blobs are copied in crtc atomic_check(), right
> before color management is checked.
>
> Changes since v1:
> - Copy all blobs immediately after drm_atomic_helper_check_modeset().
> - Clear crtc_state->hw on disable, instead of using clear_intel_crtc_state().
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_atomic.c | 44 ++++++++++++++++++
> drivers/gpu/drm/i915/display/intel_atomic.h | 2 +
> drivers/gpu/drm/i915/display/intel_display.c | 45 ++++++++++++++++---
> .../drm/i915/display/intel_display_types.h | 9 ++--
> 4 files changed, 89 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index 7cf13b9c7d38..266d0ce9d03d 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -195,6 +195,14 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>
> __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi);
>
> + /* copy color blobs */
> + if (crtc_state->hw.degamma_lut)
> + drm_property_blob_get(crtc_state->hw.degamma_lut);
> + if (crtc_state->hw.ctm)
> + drm_property_blob_get(crtc_state->hw.ctm);
> + if (crtc_state->hw.gamma_lut)
> + drm_property_blob_get(crtc_state->hw.gamma_lut);
> +
> crtc_state->update_pipe = false;
> crtc_state->disable_lp_wm = false;
> crtc_state->disable_cxsr = false;
> @@ -208,6 +216,41 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> return &crtc_state->uapi;
> }
>
> +static void intel_crtc_put_color_blobs(struct intel_crtc_state *crtc_state)
> +{
> + drm_property_blob_put(crtc_state->hw.degamma_lut);
> + drm_property_blob_put(crtc_state->hw.gamma_lut);
> + drm_property_blob_put(crtc_state->hw.ctm);
> +}
> +
> +void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state)
> +{
> + intel_crtc_put_color_blobs(crtc_state);
> +}
> +
> +void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state)
> +{
> + intel_crtc_put_color_blobs(crtc_state);
> +
> + if (crtc_state->uapi.degamma_lut)
> + crtc_state->hw.degamma_lut =
> + drm_property_blob_get(crtc_state->uapi.degamma_lut);
> + else
> + crtc_state->hw.degamma_lut = NULL;
Side note: If drm_property_blob_get() would pass NULL through this kind
of stuff would look a lot less annoying.
> +
> + if (crtc_state->uapi.gamma_lut)
> + crtc_state->hw.gamma_lut =
> + drm_property_blob_get(crtc_state->uapi.gamma_lut);
> + else
> + crtc_state->hw.gamma_lut = NULL;
> +
> + if (crtc_state->uapi.ctm)
> + crtc_state->hw.ctm =
> + drm_property_blob_get(crtc_state->uapi.ctm);
> + else
> + crtc_state->hw.ctm = NULL;
> +}
> +
> /**
> * intel_crtc_destroy_state - destroy crtc state
> * @crtc: drm crtc
> @@ -223,6 +266,7 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> struct intel_crtc_state *crtc_state = to_intel_crtc_state(state);
>
> __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
> + intel_crtc_free_hw_state(crtc_state);
> kfree(crtc_state);
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h
> index 58065d3161a3..42be91e0772a 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> @@ -35,6 +35,8 @@ intel_digital_connector_duplicate_state(struct drm_connector *connector);
> struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
> void intel_crtc_destroy_state(struct drm_crtc *crtc,
> struct drm_crtc_state *state);
> +void intel_crtc_free_hw_state(struct intel_crtc_state *crtc_state);
> +void intel_crtc_copy_color_blobs(struct intel_crtc_state *crtc_state);
> struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
> void intel_atomic_state_clear(struct drm_atomic_state *state);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 06c593d56d92..c009489641bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -114,6 +114,7 @@ static const u64 cursor_format_modifiers[] = {
> DRM_FORMAT_MOD_INVALID
> };
>
> +static void copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state);
> static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> struct intel_crtc_state *pipe_config);
> static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> @@ -7041,6 +7042,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> crtc->enabled = false;
> crtc->state->connector_mask = 0;
> crtc->state->encoder_mask = 0;
> + copy_uapi_to_hw_state(to_intel_crtc_state(crtc->state));
>
> for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
> encoder->base.crtc = NULL;
> @@ -12254,6 +12256,23 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
> return ret;
> }
>
> +static void copy_uapi_to_hw_state(struct intel_crtc_state *crtc_state)
> +{
> + crtc_state->hw.enable = crtc_state->uapi.enable;
> + crtc_state->hw.active = crtc_state->uapi.active;
> + crtc_state->hw.mode = crtc_state->uapi.mode;
> + crtc_state->hw.adjusted_mode = crtc_state->uapi.adjusted_mode;
> + intel_crtc_copy_color_blobs(crtc_state);
> +}
> +
> +static void copy_hw_to_uapi_state(struct intel_crtc_state *crtc_state)
> +{
> + crtc_state->uapi.enable = crtc_state->hw.enable;
> + crtc_state->uapi.active = crtc_state->hw.active;
> + crtc_state->uapi.mode = crtc_state->hw.mode;
> + crtc_state->uapi.adjusted_mode = crtc_state->hw.adjusted_mode;
> +}
> +
> static int
> clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> {
> @@ -12270,6 +12289,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> * fixed, so that the crtc_state can be safely duplicated. For now,
> * only fields that are know to not cause problems are preserved. */
>
> + saved_state->uapi = crtc_state->uapi;
> saved_state->scaler_state = crtc_state->scaler_state;
> saved_state->shared_dpll = crtc_state->shared_dpll;
> saved_state->dpll_hw_state = crtc_state->dpll_hw_state;
> @@ -12280,12 +12300,9 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> saved_state->wm = crtc_state->wm;
>
> - /* Keep base drm_crtc_state intact, only clear our extended struct */
> - BUILD_BUG_ON(offsetof(struct intel_crtc_state, base));
> - BUILD_BUG_ON(offsetof(struct intel_crtc_state, uapi));
> - BUILD_BUG_ON(offsetof(struct intel_crtc_state, hw));
> - memcpy(&crtc_state->uapi + 1, &saved_state->uapi + 1,
> - sizeof(*crtc_state) - sizeof(crtc_state->uapi));
> + intel_crtc_free_hw_state(crtc_state);
> + memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> + copy_uapi_to_hw_state(crtc_state);
I still think this stuff is in the wrong place. IMO all the uapi->hw copy
should happen in exactly one place (and that place I believe should be
just after drm_atomic_helper_check_modeset()). The patch changelog
seemed to imply that was what were going for but the code doesn't appear
to agree.
>
> kfree(saved_state);
> return 0;
> @@ -12425,6 +12442,9 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> DRM_DEBUG_KMS("hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
> base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>
> + /* uapi wants a copy of the adjusted_mode for vblank bookkeeping */
> + pipe_config->uapi.adjusted_mode = pipe_config->hw.adjusted_mode;
This should no longer be needed?
> +
> return 0;
> }
>
> @@ -13110,6 +13130,8 @@ verify_crtc_state(struct intel_crtc *crtc,
>
> state = old_crtc_state->uapi.state;
> __drm_atomic_helper_crtc_destroy_state(&old_crtc_state->uapi);
> + intel_crtc_free_hw_state(old_crtc_state);
> +
> pipe_config = old_crtc_state;
> memset(pipe_config, 0, sizeof(*pipe_config));
> pipe_config->uapi.crtc = &crtc->base;
> @@ -13572,11 +13594,17 @@ static int intel_atomic_check(struct drm_device *dev,
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> - if (!needs_modeset(new_crtc_state))
> + if (!needs_modeset(new_crtc_state)) {
> + if (new_crtc_state->uapi.color_mgmt_changed)
> + intel_crtc_copy_color_blobs(new_crtc_state);
> continue;
> + }
>
> if (!new_crtc_state->uapi.enable) {
> any_ms = true;
> + intel_crtc_free_hw_state(new_crtc_state);
> + memset(&new_crtc_state->hw, 0,
> + sizeof(new_crtc_state->hw));
> continue;
> }
>
> @@ -16662,6 +16690,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> to_intel_crtc_state(crtc->base.state);
>
> __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi);
> + intel_crtc_free_hw_state(crtc_state);
> memset(crtc_state, 0, sizeof(*crtc_state));
> __drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->uapi);
>
> @@ -16790,6 +16819,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> * set a flag to indicate that a full recalculation is
> * needed on the next commit.
> */
> + crtc_state->hw.mode = crtc->base.mode;
> crtc_state->hw.mode.private_flags = I915_MODE_FLAG_INHERITED;
>
> intel_crtc_compute_pixel_rate(crtc_state);
> @@ -16820,6 +16850,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
> intel_bw_crtc_update(bw_state, crtc_state);
>
> + copy_hw_to_uapi_state(crtc_state);
> intel_pipe_config_sanity_check(dev_priv, crtc_state);
> }
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 4d85ea5832d7..41d471ab9a64 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -749,8 +749,6 @@ enum intel_output_format {
> };
>
> struct intel_crtc_state {
> - union {
> - struct drm_crtc_state base;
> /*
> * uapi (drm) state. This is the software state shown to userspace.
> * In particular, the following members are used for bookkeeping:
> @@ -773,8 +771,11 @@ struct intel_crtc_state {
> *
> * During initial hw readout, they need to be copied to uapi.
> */
> - struct drm_crtc_state hw;
> - };
> + struct {
> + bool active, enable;
> + struct drm_property_blob *degamma_lut, *gamma_lut, *ctm;
> + struct drm_display_mode mode, adjusted_mode;
> + } hw;
>
> /**
> * quirks - bitfield with hw state readout quirks
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
