| Message ID | 20190920114235.22411-5-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/23] drm/i915/dp: Fix dsc bpp calculations, v2. | expand |
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. > > 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 | 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)); > Do we actually have any valid uapi state to copy at this point? I thought intel_crtc_disable_noatomic was only called during initial hardware readout (which only updates hw state and then does a hw->uapi copy at the end)? > 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); The copy only matters if crtc_state->uapi.color_mgmt_changed, right? I guess it doesn't really matter if we call this more often than we need to since we're not actually copying the blobs themselves, just dropping/taking extra references. > + > 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); Why do we need this call? uapi is preserved during this call (and then re-copied to hw), so it doesn't seem like this has any effect on the uapi/hw split we're dealing with in this patch? Matt > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 25-09-2019 om 01:41 schreef Matt Roper: > 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. >> >> 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 | 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)); >> > Do we actually have any valid uapi state to copy at this point? I > thought intel_crtc_disable_noatomic was only called during initial > hardware readout (which only updates hw state and then does a hw->uapi > copy at the end)? > > >> 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->updatenoughe_pipe || >> crtc_state->uapi.color_mgmt_changed) { >> + /* Copy color blobs to hw state */ >> + intel_crtc_copy_color_blobs(crtc_state); > The copy only matters if crtc_state->uapi.color_mgmt_changed, right? I > guess it doesn't really matter if we call this more often than we need > to since we're not actually copying the blobs themselves, just > dropping/taking extra references. Yes, exactly. :) intel_color_commit is only called when color_mgmt_changed is set, so that's sufficient. > >> + >> 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); > Why do we need this call? uapi is preserved during this call (and then > re-copied to hw), so it doesn't seem like this has any effect on the > uapi/hw split we're dealing with in this patch? We want to clear crtc_state->hw, it can be replaced with a intel_crtc_free_hw_state() + memset on crtc_state->hw. I wanted to clear the entire state, so when dumping crtc_state you don't see any old state. Although we're missing error checking here, so that needs fixing. > >> 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. > > 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 | 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. Yeah it's a tad annoying, can always just do it in this loop. We either copy full state on modeset or just the blobs.. Will send a new version.
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@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 | 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Sep 25, 2019 at 04:18:09PM +0200, Maarten Lankhorst wrote: > 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? Can't we just do something like: drm_atomic_helper_check_modeset(); for_each_crtc_in_state() { if (crtc_state->enable) copy_uapi_to_hw(); } compute_config() { if (need_bigjoiner) { get_other_crtc_state(); if (other_crtc_state->uapi.enable) fail; other_hw = current_hw; adjust_hw_state_for_bigjoiner(other_hw); } } ? > > >> 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 | 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@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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
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. 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 | 39 +++++++++++++--- .../drm/i915/display/intel_display_types.h | 8 ++-- 4 files changed, 85 insertions(+), 8 deletions(-)