Re: [PATCH v2 01/14] drm/i915: Use passed plane state for sprite planes, v3.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 03-11-15 om 10:22 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 08:31:40AM +0100, Maarten Lankhorst wrote:
>> Don't use plane->state directly, use the pointer from commit_plane.
>>
>> Changes since v1:
>> - Fix uses of plane->state->rotation and color key to use the passed state too.
>> - Only pass crtc_state and plane_state to update_plane.
>> Changes since v2:
>> - Rebased.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h    |  10 +--
>>  drivers/gpu/drm/i915/intel_sprite.c | 120 +++++++++++++++++++-----------------
>>  2 files changed, 67 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d1a6071afabe..16d4627364c5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -647,16 +647,12 @@ struct intel_plane {
>>  	/*
>>  	 * NOTE: Do not place new plane state fields here (e.g., when adding
>>  	 * new plane properties).  New runtime state should now be placed in
>> -	 * the intel_plane_state structure and accessed via drm_plane->state.
>> +	 * the intel_plane_state structure and accessed via plane_state.
>>  	 */
>>  
>>  	void (*update_plane)(struct drm_plane *plane,
>> -			     struct drm_crtc *crtc,
>> -			     struct drm_framebuffer *fb,
>> -			     int crtc_x, int crtc_y,
>> -			     unsigned int crtc_w, unsigned int crtc_h,
>> -			     uint32_t x, uint32_t y,
>> -			     uint32_t src_w, uint32_t src_h);
>> +			     struct intel_crtc_state *crtc_state,
>> +			     struct intel_plane_state *plane_state);
> These should be const.
>
>>  	void (*disable_plane)(struct drm_plane *plane,
>>  			      struct drm_crtc *crtc);
>>  	int (*check_plane)(struct drm_plane *plane,
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 4276c135b9f2..6d3047f9c80f 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
>>  }
>>  
>>  static void
>> -skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>> -		 struct drm_framebuffer *fb,
>> -		 int crtc_x, int crtc_y,
>> -		 unsigned int crtc_w, unsigned int crtc_h,
>> -		 uint32_t x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +skl_update_plane(struct drm_plane *drm_plane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = drm_plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	const int pipe = intel_plane->pipe;
>>  	const int plane = intel_plane->plane + 1;
>>  	u32 plane_ctl, stride_div, stride;
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(drm_plane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>>  	unsigned long surf_addr;
>>  	u32 tile_height, plane_offset, plane_size;
>>  	unsigned int rotation;
>>  	int x_offset, y_offset;
>> -	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
>> -	int scaler_id;
>> +
> What's with the extra newline?
>
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>> +	const struct intel_scaler *scaler =
>> +		&crtc_state->scaler_state.scalers[plane_state->scaler_id];
> This looks messy. I would at least get rid of the "assign two things on
> one line thing".
>
>>  
>>  	plane_ctl = PLANE_CTL_ENABLE |
>>  		PLANE_CTL_PIPE_GAMMA_ENABLE |
>> @@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>  	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>>  
>> -	rotation = drm_plane->state->rotation;
>> +	rotation = plane_state->base.rotation;
>>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>>  
>>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>>  					       fb->pixel_format);
>>  
>> -	scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
>> -
>>  	/* Sizes are 0 based */
>>  	src_w--;
>>  	src_h--;
>> @@ -256,13 +258,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>  	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>>  
>>  	/* program plane scaler */
>> -	if (scaler_id >= 0) {
>> +	if (plane_state->scaler_id >= 0) {
>>  		uint32_t ps_ctrl = 0;
>> +		int scaler_id = plane_state->scaler_id;
>>  
>>  		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
>>  			PS_PLANE_SEL(plane));
>> -		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
>> -			crtc_state->scaler_state.scalers[scaler_id].mode;
>> +		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) | scaler->mode;
>>  		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
>>  		I915_WRITE(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
>>  		I915_WRITE(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
>> @@ -334,24 +336,28 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>>  }
>>  
>>  static void
>> -vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>> -		 struct drm_framebuffer *fb,
>> -		 int crtc_x, int crtc_y,
>> -		 unsigned int crtc_w, unsigned int crtc_h,
>> -		 uint32_t x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +vlv_update_plane(struct drm_plane *dplane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = dplane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	int pipe = intel_plane->pipe;
>>  	int plane = intel_plane->plane;
>>  	u32 sprctl;
>>  	unsigned long sprsurf_offset, linear_offset;
>>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(dplane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> +
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>>  
>>  	sprctl = SP_ENABLE;
>>  
>> @@ -421,7 +427,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>>  							fb->pitches[0]);
>>  	linear_offset -= sprsurf_offset;
>>  
>> -	if (dplane->state->rotation == BIT(DRM_ROTATE_180)) {
>> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>>  		sprctl |= SP_ROTATE_180;
>>  
>>  		x += src_w;
>> @@ -474,23 +480,27 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>>  }
>>  
>>  static void
>> -ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>> -		 struct drm_framebuffer *fb,
>> -		 int crtc_x, int crtc_y,
>> -		 unsigned int crtc_w, unsigned int crtc_h,
>> -		 uint32_t x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +ivb_update_plane(struct drm_plane *plane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	enum pipe pipe = intel_plane->pipe;
>>  	u32 sprctl, sprscale = 0;
>>  	unsigned long sprsurf_offset, linear_offset;
>>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(plane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> +
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>>  
>>  	sprctl = SPRITE_ENABLE;
>>  
>> @@ -550,7 +560,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>  					       pixel_size, fb->pitches[0]);
>>  	linear_offset -= sprsurf_offset;
>>  
>> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
>> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>>  		sprctl |= SPRITE_ROTATE_180;
>>  
>>  		/* HSW and BDW does this automagically in hardware */
>> @@ -612,23 +622,27 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>>  }
>>  
>>  static void
>> -ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>> -		 struct drm_framebuffer *fb,
>> -		 int crtc_x, int crtc_y,
>> -		 unsigned int crtc_w, unsigned int crtc_h,
>> -		 uint32_t x, uint32_t y,
>> -		 uint32_t src_w, uint32_t src_h)
>> +ilk_update_plane(struct drm_plane *plane,
>> +		 struct intel_crtc_state *crtc_state,
>> +		 struct intel_plane_state *plane_state)
>>  {
>>  	struct drm_device *dev = plane->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>> +	struct drm_framebuffer *fb = plane_state->base.fb;
>>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  	int pipe = intel_plane->pipe;
>>  	unsigned long dvssurf_offset, linear_offset;
>>  	u32 dvscntr, dvsscale;
>>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>> -	const struct drm_intel_sprite_colorkey *key =
>> -		&to_intel_plane_state(plane->state)->ckey;
>> +	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>> +
>> +	int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +	uint32_t crtc_w = drm_rect_width(&plane_state->dst);
>> +	uint32_t crtc_h = drm_rect_height(&plane_state->dst);
>> +	uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +	uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
>> +	uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>>  
>>  	dvscntr = DVS_ENABLE;
>>  
>> @@ -684,7 +698,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>  					       pixel_size, fb->pitches[0]);
>>  	linear_offset -= dvssurf_offset;
>>  
>> -	if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
>> +	if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
>>  		dvscntr |= DVS_ROTATE_180;
>>  
>>  		x += src_w;
>> @@ -917,23 +931,17 @@ static void
>>  intel_commit_sprite_plane(struct drm_plane *plane,
>>  			  struct intel_plane_state *state)
> I wonder why that isn't const...
>
I'm guessing because until recently crtc_state wasn't really that const. :)

I agree we should constify it, but shouldn't that be in a followup patch for readability?

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx