On Thu, Oct 10, 2019 at 01:56:42PM +0200, Maarten Lankhorst wrote: > Op 08-10-2019 om 19:03 schreef Ville Syrjälä: > > On Fri, Oct 04, 2019 at 01:34:57PM +0200, Maarten Lankhorst wrote: > >> Use this in all the places where we try to acquire planes after the planes > >> atomic_check(). > >> > >> In case of intel_modeset_all_pipes() this is not yet done after > >> atomic_check, > >> but seems like it will be in the future. To add some paranoia, add all > >> planes > >> rather than active planes, because of bigjoiner and planar YUV support > >> having > >> extra planes outside of the core's view that wouldn't be added otherwise. > > If the plane isn't active what good does adding it do? > > > > Maybe the only real exception I can think of is the watermarks > > and the primary vs. gamma/csc_enable on pre-skl, but those are > > already handled correctly. > > Planar YUV Y planes are not enumerated, so it's useful to add. Instead of > typing up special case support I thought it was easier to just add all > planes. On bigjoiner slave no planes are enabled either, even if they're > active.
I think once we have the state split we should get rid of all the special casing of the Y planes. Then they (and the bigjoiner slave planes) should just look like an active planes which is not logically enabled by uapi. > > > > >> Signed-off-by: Maarten Lankhorst <[email protected]> > >> --- > >> drivers/gpu/drm/i915/display/intel_atomic.c | 41 +++++++++---------- > >> .../gpu/drm/i915/display/intel_atomic_plane.c | 19 +++++++++ > >> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 ++++--- > >> drivers/gpu/drm/i915/display/intel_color.c | 7 ++-- > >> .../drm/i915/display/intel_display_types.h | 6 +++ > >> drivers/gpu/drm/i915/intel_pm.c | 14 ++++--- > >> 6 files changed, 66 insertions(+), 36 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > >> b/drivers/gpu/drm/i915/display/intel_atomic.c > >> index c5a552a69752..e6cb85d41c8d 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > >> @@ -313,13 +313,10 @@ int intel_atomic_setup_scalers(struct > >> drm_i915_private *dev_priv, > >> struct intel_crtc *intel_crtc, > >> struct intel_crtc_state *crtc_state) > >> { > >> - struct drm_plane *plane = NULL; > >> - struct intel_plane *intel_plane; > >> - struct intel_plane_state *plane_state = NULL; > >> struct intel_crtc_scaler_state *scaler_state = > >> &crtc_state->scaler_state; > >> struct drm_atomic_state *drm_state = crtc_state->base.state; > >> - struct intel_atomic_state *intel_state = > >> to_intel_atomic_state(drm_state); > >> + struct intel_atomic_state *state = to_intel_atomic_state(drm_state); > >> int num_scalers_need; > >> int i; > >> > >> @@ -346,6 +343,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private > >> *dev_priv, > >> > >> /* walkthrough scaler_users bits and start assigning scalers */ > >> for (i = 0; i < sizeof(scaler_state->scaler_users) * 8; i++) { > >> + struct intel_plane_state *plane_state = NULL; > >> int *scaler_id; > >> const char *name; > >> int idx; > >> @@ -361,19 +359,16 @@ int intel_atomic_setup_scalers(struct > >> drm_i915_private *dev_priv, > >> /* panel fitter case: assign as a crtc scaler */ > >> scaler_id = &scaler_state->scaler_id; > >> } else { > >> - name = "PLANE"; > >> + struct intel_plane *plane; > >> > >> /* plane scaler case: assign as a plane scaler */ > >> /* find the plane that set the bit as scaler_user */ > >> - plane = drm_state->planes[i].ptr; > >> > >> /* > >> * to enable/disable hq mode, add planes that are using > >> scaler > >> * into this transaction > >> */ > >> - if (!plane) { > >> - struct drm_plane_state *state; > >> - > >> + if (!drm_state->planes[i].ptr) { > >> /* > >> * GLK+ scalers don't have a HQ mode so it > >> * isn't necessary to change between HQ and dyn > >> mode > >> @@ -382,24 +377,28 @@ int intel_atomic_setup_scalers(struct > >> drm_i915_private *dev_priv, > >> if (INTEL_GEN(dev_priv) >= 10 || > >> IS_GEMINILAKE(dev_priv)) > >> continue; > >> > >> - plane = drm_plane_from_index(&dev_priv->drm, i); > >> - state = drm_atomic_get_plane_state(drm_state, > >> plane); > >> - if (IS_ERR(state)) { > >> - DRM_DEBUG_KMS("Failed to add [PLANE:%d] > >> to drm_state\n", > >> - plane->base.id); > >> - return PTR_ERR(state); > >> + plane = > >> to_intel_plane(drm_plane_from_index(&dev_priv->drm, i)); > >> + plane_state = > >> + > >> intel_atomic_get_plane_state_after_check(state, > >> + > >> crtc_state, > >> + > >> plane); > >> + if (IS_ERR(plane_state)) { > >> + DRM_DEBUG_KMS("Failed to add [PLANE:%d] > >> to drm_state: %li\n", > >> + plane->base.base.id, > >> PTR_ERR(plane_state)); > >> + return PTR_ERR(plane_state); > >> } > >> + } else { > >> + plane = > >> to_intel_plane(drm_state->planes[i].ptr); > >> + plane_state = > >> intel_atomic_get_new_plane_state(state, > >> + > >> plane); > >> } > >> > >> - intel_plane = to_intel_plane(plane); > >> - idx = plane->base.id; > >> - > >> /* plane on different crtc cannot be a scaler user of > >> this crtc */ > >> - if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) > >> + if (WARN_ON(plane->pipe != intel_crtc->pipe)) > >> continue; > >> > >> - plane_state = > >> intel_atomic_get_new_plane_state(intel_state, > >> - > >> intel_plane); > >> + name = "PLANE"; > >> + idx = plane->base.base.id; > >> scaler_id = &plane_state->scaler_id; > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> index 98b7766eaa7a..4eaab0de98bf 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > >> @@ -366,6 +366,25 @@ void i9xx_update_planes_on_crtc(struct > >> intel_atomic_state *state, > >> } > >> } > >> > >> +struct intel_plane_state * > >> +intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, > >> + struct intel_crtc_state > >> *new_crtc_state, > >> + struct intel_plane *plane) > >> +{ > >> + struct intel_plane_state *plane_state = > >> + intel_atomic_get_new_plane_state(state, plane); > >> + > >> + if (plane_state) > >> + return plane_state; > >> + > >> + plane_state = intel_atomic_get_plane_state(state, plane); > >> + if (IS_ERR(plane_state)) > >> + return plane_state; > >> + > >> + new_crtc_state->update_planes |= BIT(plane->id); > > I also don't really like burying this all the way down. Seems > > rather non-obvious from the caller. Also seems wrong to not set > > the flag if the plane was already part of the state. > > > > I'd say we just need to hand roll add_affected_planes() for the > > cdclk code. And there is should definitely be sufficient to > > add just the active planes. > > > >> + return plane_state; > >> +} > >> + > >> const struct drm_plane_helper_funcs intel_plane_helper_funcs = { > >> .prepare_fb = intel_prepare_plane_fb, > >> .cleanup_fb = intel_cleanup_plane_fb, > >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > >> b/drivers/gpu/drm/i915/display/intel_cdclk.c > >> index 43564295b864..42bd02638d32 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > >> @@ -2249,6 +2249,7 @@ static int intel_modeset_all_pipes(struct > >> intel_atomic_state *state) > >> */ > >> for_each_intel_crtc(&dev_priv->drm, crtc) { > >> struct intel_crtc_state *crtc_state; > >> + struct intel_plane *plane; > >> int ret; > >> > >> crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); > >> @@ -2266,12 +2267,14 @@ static int intel_modeset_all_pipes(struct > >> intel_atomic_state *state) > >> if (ret) > >> return ret; > >> > >> - ret = drm_atomic_add_affected_planes(&state->base, > >> - &crtc->base); > >> - if (ret) > >> - return ret; > >> - > >> - crtc_state->update_planes |= crtc_state->active_planes; > >> + for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) { > >> + struct intel_plane_state *plane_state = > >> + intel_atomic_get_plane_state_after_check(state, > >> + > >> crtc_state, > >> + plane); > >> + if (IS_ERR(plane_state)) > >> + return PTR_ERR(plane_state); > >> + } > >> } > >> > >> return 0; > >> diff --git a/drivers/gpu/drm/i915/display/intel_color.c > >> b/drivers/gpu/drm/i915/display/intel_color.c > >> index 9ab34902663e..1e3a623eaf82 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_color.c > >> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >> @@ -1077,11 +1077,12 @@ intel_color_add_affected_planes(struct > >> intel_crtc_state *new_crtc_state) > >> if (!need_plane_update(plane, new_crtc_state)) > >> continue; > >> > >> - plane_state = intel_atomic_get_plane_state(state, plane); > >> + plane_state = > >> + intel_atomic_get_plane_state_after_check(state, > >> + new_crtc_state, > >> + plane); > >> if (IS_ERR(plane_state)) > >> return PTR_ERR(plane_state); > >> - > >> - new_crtc_state->update_planes |= BIT(plane->id); > >> } > >> > >> return 0; > >> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > >> b/drivers/gpu/drm/i915/display/intel_display_types.h > >> index 976669f01a8c..526423437f63 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h > >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > >> @@ -1541,4 +1541,10 @@ static inline u32 intel_plane_ggtt_offset(const > >> struct intel_plane_state *state) > >> return i915_ggtt_offset(state->vma); > >> } > >> > >> +/* intel_atomic_plane.c */ > >> +struct intel_plane_state * > >> +intel_atomic_get_plane_state_after_check(struct intel_atomic_state *state, > >> + struct intel_crtc_state *crtc_state, > >> + struct intel_plane *plane); > >> + > >> #endif /* __INTEL_DISPLAY_TYPES_H__ */ > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c > >> b/drivers/gpu/drm/i915/intel_pm.c > >> index 53358e33df1b..5e6e54cb22fe 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -5242,11 +5242,12 @@ skl_ddb_add_affected_planes(const struct > >> intel_crtc_state *old_crtc_state, > >> > >> &new_crtc_state->wm.skl.plane_ddb_uv[plane_id])) > >> continue; > >> > >> - plane_state = intel_atomic_get_plane_state(state, plane); > >> + plane_state = > >> + intel_atomic_get_plane_state_after_check(state, > >> + new_crtc_state, > >> + plane); > >> if (IS_ERR(plane_state)) > >> return PTR_ERR(plane_state); > >> - > >> - new_crtc_state->update_planes |= BIT(plane_id); > >> } > >> > >> return 0; > >> @@ -5534,11 +5535,12 @@ static int skl_wm_add_affected_planes(struct > >> intel_atomic_state *state, > >> > >> &new_crtc_state->wm.skl.optimal.planes[plane_id])) > >> continue; > >> > >> - plane_state = intel_atomic_get_plane_state(state, plane); > >> + plane_state = > >> + intel_atomic_get_plane_state_after_check(state, > >> + new_crtc_state, > >> + plane); > >> if (IS_ERR(plane_state)) > >> return PTR_ERR(plane_state); > >> - > >> - new_crtc_state->update_planes |= BIT(plane_id); > >> } > >> > >> return 0; > >> -- > >> 2.23.0 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> [email protected] > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
