From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 08/14] drm/i915: Complete crtc hw/uapi split, v2.
Date: Wed, 23 Oct 2019 17:06:16 +0200 [thread overview]
Message-ID: <46ac6971-3936-b121-7723-ced1114e5cda@linux.intel.com> (raw)
In-Reply-To: <20191022183238.GS1208@intel.com>
Op 22-10-2019 om 20:32 schreef Ville Syrjälä:
> 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@linux.intel.com>
>> ---
>> 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.
Yeah I will just make a single function on top of there.
>
>>
>> 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?
Yeah, think so. One way to find out. :)
>
>> +
>> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
WARNING: multiple messages have this Message-ID (diff)
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 08/14] drm/i915: Complete crtc hw/uapi split, v2.
Date: Wed, 23 Oct 2019 17:06:16 +0200 [thread overview]
Message-ID: <46ac6971-3936-b121-7723-ced1114e5cda@linux.intel.com> (raw)
Message-ID: <20191023150616.8mdlhuuP2PmrNVchEF9BjqV3Qt_QBj73TFBUX5rk6E4@z> (raw)
In-Reply-To: <20191022183238.GS1208@intel.com>
Op 22-10-2019 om 20:32 schreef Ville Syrjälä:
> 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@linux.intel.com>
>> ---
>> 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.
Yeah I will just make a single function on top of there.
>
>>
>> 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?
Yeah, think so. One way to find out. :)
>
>> +
>> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-10-23 15:06 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-17 13:20 [PATCH 01/14] drm/i915: Rework watermark readout to use plane api Maarten Lankhorst
2019-10-17 13:20 ` [PATCH 02/14] drm/i915: Introduce intel_atomic_get_plane_state_after_check(), v2 Maarten Lankhorst
2019-10-17 13:20 ` [PATCH 03/14] drm/i915: Handle a few more cases for crtc hw/uapi split, v2 Maarten Lankhorst
2019-10-18 10:33 ` Ville Syrjälä
2019-10-17 13:20 ` [PATCH 04/14] drm/i915: Add aliases for uapi and hw to crtc_state Maarten Lankhorst
2019-10-18 10:36 ` Ville Syrjälä
2019-10-18 12:09 ` Maarten Lankhorst
2019-10-18 13:09 ` Ville Syrjälä
2019-10-17 13:20 ` [PATCH 05/14] drm/i915: Perform manual conversions for crtc uapi/hw split Maarten Lankhorst
2019-10-18 8:11 ` Maarten Lankhorst
2019-10-18 8:13 ` [PATCH] drm/i915: Perform manual conversions for crtc uapi/hw split, v2 Maarten Lankhorst
2019-10-22 18:16 ` Ville Syrjälä
2019-10-24 12:12 ` Maarten Lankhorst
2019-10-24 12:12 ` [Intel-gfx] " Maarten Lankhorst
2019-10-24 12:23 ` Ville Syrjälä
2019-10-24 12:23 ` [Intel-gfx] " Ville Syrjälä
2019-10-24 12:35 ` Maarten Lankhorst
2019-10-24 12:35 ` [Intel-gfx] " Maarten Lankhorst
2019-10-17 13:20 ` [PATCH 06/14] drm/i915: Perform automated conversions for crtc uapi/hw split, base -> hw Maarten Lankhorst
2019-10-18 12:59 ` Ville Syrjälä
2019-10-17 13:20 ` [PATCH 07/14] drm/i915: Perform automated conversions for crtc uapi/hw split, base -> uapi Maarten Lankhorst
2019-10-22 18:24 ` Ville Syrjälä
2019-10-17 13:20 ` [PATCH 08/14] drm/i915: Complete crtc hw/uapi split, v2 Maarten Lankhorst
2019-10-22 18:32 ` Ville Syrjälä
2019-10-23 15:06 ` Maarten Lankhorst [this message]
2019-10-23 15:06 ` [Intel-gfx] " Maarten Lankhorst
2019-10-17 13:21 ` [PATCH 09/14] drm/i915: Add aliases for uapi and hw to plane_state Maarten Lankhorst
2019-10-17 13:21 ` [PATCH 10/14] drm/i915: Perform manual conversions for plane uapi/hw split Maarten Lankhorst
2019-10-17 13:21 ` [PATCH 11/14] drm/i915: Perform automated conversions for plane uapi/hw split, base -> hw Maarten Lankhorst
2019-10-17 13:21 ` [PATCH 12/14] drm/i915: Perform automated conversions for plane uapi/hw split, base -> uapi Maarten Lankhorst
2019-10-17 13:21 ` [PATCH 13/14] drm/i915: Complete plane hw and uapi split, v2 Maarten Lankhorst
2019-10-17 13:21 ` [PATCH 14/14] drm/i915: Remove special case slave handling during hw programming, v2 Maarten Lankhorst
2019-10-18 14:52 ` Maarten Lankhorst
2019-10-22 10:31 ` [PATCH] drm/i915: Remove special case slave handling during hw programming, v3 Maarten Lankhorst
2019-10-22 18:14 ` Ville Syrjälä
2019-10-23 10:31 ` Maarten Lankhorst
2019-10-23 10:31 ` [Intel-gfx] " Maarten Lankhorst
2019-10-17 13:37 ` [PATCH 01/14] drm/i915: Rework watermark readout to use plane api Ville Syrjälä
2019-10-17 14:11 ` Maarten Lankhorst
2019-10-17 18:25 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/14] " Patchwork
2019-10-17 18:50 ` ✗ Fi.CI.BAT: failure " Patchwork
2019-10-18 8:27 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/14] drm/i915: Rework watermark readout to use plane api (rev2) Patchwork
2019-10-18 8:46 ` ✓ Fi.CI.BAT: success " Patchwork
2019-10-18 10:57 ` ✗ Fi.CI.IGT: failure " Patchwork
2019-10-18 14:03 ` Maarten Lankhorst
2019-10-22 18:53 ` ✗ Fi.CI.BUILD: failure for series starting with [01/14] drm/i915: Rework watermark readout to use plane api (rev3) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=46ac6971-3936-b121-7723-ced1114e5cda@linux.intel.com \
--to=maarten.lankhorst@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox