| Message ID | 20191017132105.15528-8-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] drm/i915: Rework watermark readout to use plane api | expand |
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. > > 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? > + > 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
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
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; + + 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); 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; + 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
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(-)