On Tue, Oct 29, 2019 at 04:33:15PM +0200, Ville Syrjälä wrote:
> On Tue, Oct 29, 2019 at 08:22:24AM +0100, 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().
> > Changes since v2:
> > - Use intel_crtc_free_hw_state + clear in intel_crtc_disable_noatomic().
> > - Make a intel_crtc_prepare_state() function that clears the crtc_state
> > and copies hw members.
> > - Remove setting uapi.adjusted_mode, we now have a direct call to
> > drm_calc_timestamping_constants().
> > Changes since v3:
> > - Rename prefix copy_hw_to_uapi_state() with intel_crtc.
> > - Copy color blobs to uapi as well.
> > - Add a intel_crtc_copy_uapi_to_hw_state_nomodeset() function for clarity.
> >
> > 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 | 80 ++++++++++++++++---
> > .../drm/i915/display/intel_display_types.h | 9 ++-
> > 4 files changed, 118 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index 366275dc113d..557178906ccf 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;
> > +
> > + 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 49d5cb1b9e0a..7b49623419ba 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.h
> > @@ -36,6 +36,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 1bc008c094e4..a765794597ed 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7185,6 +7185,8 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > crtc->enabled = false;
> > crtc->state->connector_mask = 0;
> > crtc->state->encoder_mask = 0;
> > + intel_crtc_free_hw_state(crtc_state);
> > + memset(&crtc_state->hw, 0, sizeof(crtc_state->hw));
> >
> > for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
> > encoder->base.crtc = NULL;
> > @@ -12560,8 +12562,48 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
> > return ret;
> > }
> >
> > +static void
> > +intel_crtc_copy_uapi_to_hw_state_nomodeset(struct intel_crtc_state *crtc_state)
> > +{
> > + intel_crtc_copy_color_blobs(crtc_state);
> > +}
> > +
> > +static void
> > +intel_crtc_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_uapi_to_hw_state_nomodeset(crtc_state);
> > +}
> > +
> > +static void intel_crtc_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;
> > +
> > + /* copy color blobs to uapi */
> > + drm_property_blob_put(crtc_state->hw.degamma_lut);
>
> uapi.degamma_lut
>
> These look like inlined versions of drm_property_replace_blob()
> actually.
And seems to me that you should be able to use that for the
other direction as well.
>
> Otherwise lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> > + crtc_state->uapi.degamma_lut = crtc_state->hw.degamma_lut;
> > + if (crtc_state->uapi.degamma_lut)
> > + drm_property_blob_get(crtc_state->uapi.degamma_lut);
> > +
> > + drm_property_blob_put(crtc_state->uapi.ctm);
> > + crtc_state->uapi.ctm = crtc_state->hw.ctm;
> > + if (crtc_state->uapi.ctm)
> > + drm_property_blob_get(crtc_state->uapi.ctm);
> > +
> > + drm_property_blob_put(crtc_state->uapi.gamma_lut);
> > + crtc_state->uapi.gamma_lut = crtc_state->hw.gamma_lut;
> > + if (crtc_state->uapi.gamma_lut)
> > + drm_property_blob_get(crtc_state->uapi.gamma_lut);
> > +}
> > +
> > static int
> > -clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > +intel_crtc_prepare_cleared_state(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv =
> > to_i915(crtc_state->uapi.crtc->dev);
> > @@ -12571,11 +12613,15 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > if (!saved_state)
> > return -ENOMEM;
> >
> > + /* free the old crtc_state->hw members */
> > + intel_crtc_free_hw_state(crtc_state);
> > +
> > /* FIXME: before the switch to atomic started, a new pipe_config was
> > * kzalloc'd. Code that depends on any field being zero should be
> > * 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;
> > @@ -12593,14 +12639,11 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
> > saved_state->sync_mode_slaves_mask =
> > crtc_state->sync_mode_slaves_mask;
> >
> > - /* 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));
> > -
> > + memcpy(crtc_state, saved_state, sizeof(*crtc_state));
> > kfree(saved_state);
> > +
> > + intel_crtc_copy_uapi_to_hw_state(crtc_state);
> > +
> > return 0;
> > }
> >
> > @@ -12616,10 +12659,6 @@ intel_modeset_pipe_config(struct intel_crtc_state *pipe_config)
> > int i;
> > bool retry = true;
> >
> > - ret = clear_intel_crtc_state(pipe_config);
> > - if (ret)
> > - return ret;
> > -
> > pipe_config->cpu_transcoder =
> > (enum transcoder) to_intel_crtc(crtc)->pipe;
> >
> > @@ -13476,6 +13515,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;
> > @@ -14010,14 +14051,24 @@ 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)) {
> > + /* Light copy */
> > + intel_crtc_copy_uapi_to_hw_state_nomodeset(new_crtc_state);
> > +
> > continue;
> > + }
> >
> > if (!new_crtc_state->uapi.enable) {
> > + intel_crtc_copy_uapi_to_hw_state(new_crtc_state);
> > +
> > any_ms = true;
> > continue;
> > }
> >
> > + ret = intel_crtc_prepare_cleared_state(new_crtc_state);
> > + if (ret)
> > + goto fail;
> > +
> > ret = intel_modeset_pipe_config(new_crtc_state);
> > if (ret)
> > goto fail;
> > @@ -17291,6 +17342,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);
> >
> > @@ -17419,6 +17471,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);
> > @@ -17467,6 +17520,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >
> > intel_bw_crtc_update(bw_state, crtc_state);
> >
> > + intel_crtc_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 573ced9dc909..bcf5f7640307 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -757,8 +757,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:
> > @@ -781,8 +779,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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
