Op 25-09-2019 om 15:01 schreef Ville Syrjälä:
> On Fri, Sep 20, 2019 at 01:42:17PM +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.
> Don't like. IMO there should be one and only one place where copy over
> the uapi state into the hw state. Otherwise it's a mess because you
> constantly have to remind yourself which bits of state already got
> copied and which didn't.
Oh now I remember, because bigjoiner is assigned later on we can't copy blobs yet
and bigjoiner was the whole reaon we wanted separate blobs so it didn't have to look at the other crtc_state..
Would it help if I copy the blobs in intel_color_check() instead?
>> 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 | 39 +++++++++++++---
>> .../drm/i915/display/intel_display_types.h | 8 ++--
>> 4 files changed, 85 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
>> index f4440ede95c5..fb550d3cea7f 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;
>> @@ -209,6 +217,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
>> @@ -224,6 +267,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 32bbb5bf48f3..e40485a1e503 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,
>> @@ -7097,6 +7098,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;
>> @@ -11804,6 +11806,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *_crtc,
>>
>> if (mode_changed || crtc_state->update_pipe ||
>> crtc_state->uapi.color_mgmt_changed) {
>> + /* Copy color blobs to hw state */
>> + intel_crtc_copy_color_blobs(crtc_state);
>> +
>> ret = intel_color_check(crtc_state);
>> if (ret)
>> return ret;
>> @@ -12251,6 +12256,22 @@ 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;
>> +}
>> +
>> +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)
>> {
>> @@ -12267,6 +12288,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;
>> @@ -12277,11 +12299,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, 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);
>>
>> kfree(saved_state);
>> return 0;
>> @@ -12421,6 +12441,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;
>> +
>> return 0;
>> }
>>
>> @@ -13142,6 +13165,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;
>> @@ -13568,6 +13593,7 @@ static int intel_atomic_check(struct drm_device *dev,
>>
>> if (!new_crtc_state->uapi.enable) {
>> any_ms = true;
>> + clear_intel_crtc_state(new_crtc_state);
>> continue;
>> }
>>
>> @@ -16686,6 +16712,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);
>>
>> @@ -16802,6 +16829,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>> crtc->base.mode.vdisplay = crtc_state->pipe_src_h;
>> intel_mode_from_pipe_config(&crtc_state->hw.adjusted_mode,
>> crtc_state);
>> + crtc_state->hw.mode = crtc->base.mode;
>> WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
>>
>> /*
>> @@ -16847,6 +16875,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 2c3567081e16..e81b785cc8f2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -749,7 +749,6 @@ enum intel_output_format {
>> };
>>
>> struct intel_crtc_state {
>> - union {
>> /*
>> * uapi (drm) state. This is the software state shown to userspace.
>> * In particular, the following members are used for bookkeeping:
>> @@ -772,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.20.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
