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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 10, 2017 at 11:04:31AM +0200, Maarten Lankhorst wrote:
> Op 06-07-17 om 22:24 schreef ville.syrjala@xxxxxxxxxxxxxxx:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> >  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?

I was trying to keep these patches as minimal as possible. So I was
avoiding renaming things that already had names. I do agree whole
heartedly that we have to rename these things but I was thinking we
could perhaps try to do that with coccinelle. Not quite sure if that's
feasible but seems like an interesting thing to try out.

> >  {
> > -	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?

Yes. In this case that definitely makes sense since it's not used elsewhere.

> > -	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.

I guess this function is small enough that doing the renames immediately
does make sense.

> 
> 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. :)

The code is already pretty much unreadable in places due to this.
Hence I was aiming for doing a mass rename later.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux