Op 24-10-2019 om 17:21 schreef Ville Syrjälä:
> On Thu, Oct 24, 2019 at 02:47: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().
>> 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().
>>
>> 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 | 56 +++++++++++++++----
>> .../drm/i915/display/intel_display_types.h | 9 +--
>> 4 files changed, 95 insertions(+), 16 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)
> This is only used in intel_display.c so should perhaps live there?
>
>> +{
>> + 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 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 11dd7a182543..2dbc1df9505a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -7110,6 +7110,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;
>> @@ -12482,22 +12484,50 @@ static bool check_digital_port_conflicts(struct intel_atomic_state *state)
>> return ret;
>> }
>>
>> +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_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;
> Why are we not copying the color blobs?
Hmm, we probably should at least copy the gamma blob, but probably all. :)
Missed that because this patch lingered on the ml for so long we added hw readout in the mean time.
>
>> +}
>> +
>> static int
>> -clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>> +intel_crtc_prepare_state(struct intel_crtc_state *crtc_state)
> Hmm. I was hoping we could make this even simpler, but maybe I'm a bit
> naive. I guess we really do need to consider needs_modeset() so as to
> not clobber the already computed hw state when we're not going to take
> the full .compute_config() path.
>
>> {
>> struct drm_i915_private *dev_priv =
>> to_i915(crtc_state->uapi.crtc->dev);
>> struct intel_crtc_state *saved_state;
>>
>> + if (!needs_modeset(crtc_state)) {
>> + /* Only need to update crtc_state->hw color blobs */
> That comment is likely to get out of date. Not sure how to word it in a
> nice way though. Essentially we just want to copy the part of the state
> that doesn't clobber anything set up by .compute_config() & co.
>
> It would be nice if we had the same level of abstraction for this case
> as for the full modeset case. Eg. intel_crtc_copy_hw_state_non_modeset()
> or something. A bit ugly that name though. Might have think of something
> better.
>
> Another thing that's a bit confusing is that we need to do the
> intel_crtc_free_hw_state() for the full modeset path, but we don't
> need it for the non-modeset path. Would be nice it the two paths
> behaved more uniformly.
We'd have to get rid of the crtc_state clearing for that. :(
>> + intel_crtc_copy_color_blobs(crtc_state);
>> + return 0;
>> + }
>> +
>> saved_state = kzalloc(sizeof(*saved_state), GFP_KERNEL);
>> 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;
>> @@ -12515,14 +12545,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;
>> }
>>
>> @@ -12538,10 +12565,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;
>>
>> @@ -13361,6 +13384,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;
>> @@ -13823,6 +13848,10 @@ 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) {
>> + ret = intel_crtc_prepare_state(new_crtc_state);
>> + if (ret)
>> + return ret;
> I have a feeling this should be its own loop in case we need
> to insert global stuff after this.
>
> For example, I think the port sync stuff should be there but
> I can't see it now. Ah, for some reason it's buried deep inside
> intel_modeset_pipe_config() which seems wrong. If any (or at
> least the master) of the genlocked pipes does a full modeset
> we need to do a full modeset on all of them, which should also
> mean a full .compute_config() on everything.
>
> Hmm. And if we do that then combining the uapi->hw copy and
> clear_intel_crtc_state() like this is not going to work.
Genlock would be messy either way, with fastset vs modeset. It would need its own second loop to handle correctly.
>
>> +
>> if (!needs_modeset(new_crtc_state))
>> continue;
>>
>> @@ -17099,6 +17128,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);
>>
>> @@ -17227,6 +17257,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);
>> @@ -17257,6 +17288,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 4697f719b3a8..17c9ec300f18 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
