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.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> 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
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
