Re: [Intel-gfx] [PATCH 04/22] drm/i915: Pass proper old/new states to intel_plane_atomic_check_with_state()

Op 06-07-17 om 22:24 schreef ville.syrjala@linux.intel.com:

From: Ville Syrjälä ville.syrjala@linux.intel.com

Eliminate plane->state and crtc->state usage from intel_plane_atomic_check_with_state() and its callers. Instead pass the proper states in or dig them up from the top level atomic state.

Note that intel_plane_atomic_check_with_state() itself isn't allowed to use the top level atomic state as there is none when it gets called from the legacy cursor short circuit path.

Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com

drivers/gpu/drm/i915/intel_atomic_plane.c | 40 +++++++++++++++++++------------ drivers/gpu/drm/i915/intel_display.c | 12 ++++++---- drivers/gpu/drm/i915/intel_drv.h | 16 +++++++++++-- 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index ee76fab7bb6f..7cdbe9ae2c96 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -107,7 +107,9 @@ intel_plane_destroy_state(struct drm_plane *plane, drm_atomic_helper_plane_destroy_state(plane, state); }

-int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, +int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,

• 			struct intel_crtc_state *crtc_state,
  

• 			const struct intel_plane_state *old_plane_state,
  		struct intel_plane_state *intel_state)
  

{ struct drm_plane *plane = intel_state->base.plane; @@ -124,7 +126,7 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, * anything driver-specific we need to test in that case, so * just return success. */

• if (!intel_state->base.crtc && !plane->state->crtc)

• if (!intel_state->base.crtc && !old_plane_state->base.crtc) return 0;

  /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */

@@ -194,17 +196,21 @@ int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, else crtc_state->active_planes &= ~BIT(intel_plane->id);

• return intel_plane_atomic_calc_changes(&crtc_state->base, state);

• return intel_plane_atomic_calc_changes(old_crtc_state,

• 			       &crtc_state->base,
  

• 			       old_plane_state,
  

• 			       state);
  

}

static int intel_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state)

new_plane_state?

{

• struct drm_crtc *crtc = state->crtc;

• const struct drm_plane_state *old_plane_state =

• drm_atomic_get_old_plane_state(state->state, plane);
  

• struct drm_crtc *crtc = state->crtc ?: old_plane_state->crtc;
• const struct drm_crtc_state *old_crtc_state; struct drm_crtc_state *drm_crtc_state;

• crtc = crtc ? crtc : plane->state->crtc;
• /*
  • Both crtc and plane->crtc could be NULL if we're updating a
  • property while the plane is disabled. We don't actually have

@@ -214,29 +220,33 @@ static int intel_plane_atomic_check(struct drm_plane *plane, if (!crtc) return 0;

• drm_crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
• if (WARN_ON(!drm_crtc_state))

• return -EINVAL;
  

• old_crtc_state = drm_atomic_get_old_crtc_state(state->state, crtc);
• drm_crtc_state = drm_atomic_get_new_crtc_state(state->state, crtc);

new_crtc_state?

• return intel_plane_atomic_check_with_state(to_intel_crtc_state(drm_crtc_state),

• return intel_plane_atomic_check_with_state(to_intel_crtc_state(old_crtc_state),

• 				   to_intel_crtc_state(drm_crtc_state),
  

• 				   to_intel_plane_state(old_plane_state),
  			   to_intel_plane_state(state));
  

}

static void intel_plane_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) {

• struct intel_atomic_state *state = to_intel_atomic_state(old_state->state); struct intel_plane *intel_plane = to_intel_plane(plane);

• struct intel_plane_state *intel_state =

• to_intel_plane_state(plane->state);
  

• struct drm_crtc *crtc = plane->state->crtc ?: old_state->crtc;

• const struct intel_plane_state *intel_state =

• intel_atomic_get_new_plane_state(state, intel_plane);
  

• struct drm_crtc *crtc = intel_state->base.crtc ?: old_state->crtc;

  if (intel_state->base.visible) {

• const struct intel_crtc_state *intel_crtc_state =
  

• 	intel_atomic_get_new_crtc_state(state, to_intel_crtc(crtc));
  

• trace_intel_update_plane(plane, to_intel_crtc(crtc));

  intel_plane->update_plane(intel_plane,

• 			  to_intel_crtc_state(crtc->state),
  

• 			  intel_state);
  

• 			  intel_crtc_state, intel_state);
  

  } else { trace_intel_disable_plane(plane, to_intel_crtc(crtc));

I think with so many names, might be better to use new_crtc_state and new_plane_state here.

State is used so much as keyword in atomic that with multiple states, intel_state becomes as descriptive as void *ptr. :)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cdfa95be4b8e..6440479d6fe2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11011,7 +11011,7 @@ static bool intel_wm_need_update(struct drm_plane *plane, return false; }

-static bool needs_scaling(struct intel_plane_state *state) +static bool needs_scaling(const struct intel_plane_state *state) { int src_w = drm_rect_width(&state->base.src) >> 16; int src_h = drm_rect_height(&state->base.src) >> 16; @@ -11021,7 +11021,9 @@ static bool needs_scaling(struct intel_plane_state *state) return (src_w != dst_w || src_h != dst_h); }

-int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, +int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,

• 		    struct drm_crtc_state *crtc_state,
  

• 		    const struct intel_plane_state *old_plane_state,
  	    struct drm_plane_state *plane_state)
  

{ struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state); @@ -11030,10 +11032,8 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, struct intel_plane *plane = to_intel_plane(plane_state->plane); struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = to_i915(dev);

• struct intel_plane_state *old_plane_state =

• to_intel_plane_state(plane->base.state);
  

  bool mode_changed = needs_modeset(crtc_state);
• bool was_crtc_enabled = crtc->state->active;

• bool was_crtc_enabled = old_crtc_state->base.active; bool is_crtc_enabled = crtc_state->active; bool turn_off, turn_on, visible, was_visible; struct drm_framebuffer *fb = plane_state->fb;

@@ -13660,6 +13660,8 @@ intel_legacy_cursor_update(struct drm_plane *plane, new_plane_state->crtc_h = crtc_h;

ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),

^old crtc state here?

• 				  to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
  

and crtc_state here? You already pass both to this function?

• 				  to_intel_plane_state(plane->state),
  

and old_plane_state here?

				  to_intel_plane_state(new_plane_state));

if (ret) goto out_free; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e9d61a03c46e..ea36d1a61e86 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1207,6 +1207,14 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi) return container_of(intel_hdmi, struct intel_digital_port, hdmi); }

+static inline struct intel_plane_state * +intel_atomic_get_new_plane_state(struct intel_atomic_state *state,

• 		 struct intel_plane *plane)
  

+{

• return to_intel_plane_state(drm_atomic_get_new_plane_state(&state->base,

• 						   &plane->base));
  

+}

static inline struct intel_crtc_state * intel_atomic_get_old_crtc_state(struct intel_atomic_state *state, struct intel_crtc *crtc) @@ -1439,7 +1447,9 @@ int intel_plane_atomic_set_property(struct drm_plane *plane, struct drm_plane_state *state, struct drm_property *property, uint64_t val); -int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, +int intel_plane_atomic_calc_changes(const struct intel_crtc_state *old_crtc_state,

• 		    struct drm_crtc_state *crtc_state,
  

• 		    const struct intel_plane_state *old_plane_state,
  	    struct drm_plane_state *plane_state);
  
  

void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv, @@ -1990,7 +2000,9 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane); void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state); extern const struct drm_plane_helper_funcs intel_plane_helper_funcs; -int intel_plane_atomic_check_with_state(struct intel_crtc_state *crtc_state, +int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,

• 			struct intel_crtc_state *crtc_state,
  

• 			const struct intel_plane_state *old_plane_state,
  		struct intel_plane_state *intel_state);
  
  

/* intel_color.c */

Please for this whole patch, use old/new_plane/crtc_state, or it becomes unreadable very fast. :)

Idea looks good though, so looking forward to v2.