On Fri, Oct 25, 2019 at 11:00:06AM +0200, Maarten Lankhorst wrote:
> 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. :(
I was just thinking we'd have something like:
crtc_state_free_lite() + crtc_state_copy_lite()
and
crtc_state_free_full() + crtc_state_copy_full()
With better names probably :)
Then both paths would do the same kind of free+copy and the reader
wouldn't have to wonder why they look so different.
> >> + 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.
Hmm. Yeah, this fastset vs. port sync is particularly troublesome
because we now copy some state back from the old state.
Not really sure what to do about that other than:
a) don't do fastset with port sync (would be a shame)
b) redo .compute_config() for all tiles if at least one of
them still insists on a full modeset after all have gone
through the fastset check (a bit yucky)
c) copy even more state to make sure it's all totally consistent
(should really do this anyway), and then just force a modeset
on all tiles if one needs it but without redoing any
.compute_config() stuff (ie. do the modeset with the fastset
adjusted state)
I maybe like c) best. b) would perhaps allow us to not do any port sync
things before the .compute_config() loop since we'd redo it anyway if
anything affected neds a modeset. But having a retry loop would be a
little annoying. Although we're going to need some kind of retry loop
for eg. MST bandwidth vs. bpc stuff anyway.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
