[Intel-gfx] [PATCH 08/21 v2] drm/i915: Add helper function to update scaler_users in crtc_state

[Konduru, Chandra]

> -----Original Message-----
> From: Roper, Matthew D
> Sent: Tuesday, March 24, 2015 10:15 PM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 08/21 v2] drm/i915: Add helper function to update
> scaler_users in crtc_state
> 
> On Fri, Mar 20, 2015 at 05:04:29PM -0700, Chandra Konduru wrote:
> > This helper function stages a scaler request for a plane/crtc into
> > crtc_state->scaler_users (which is a bit field). It also performs
> > required checks before staging any change into scaler_state.
> >
> > v2:
> > -updates to use single copy of scaler limits (Matt) -added force
> > detach parameter for pfit disable purpose (me)
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  143
> ++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |    3 +
> >  2 files changed, 146 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 890d372..976bfb1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12387,6 +12387,149 @@ intel_cleanup_plane_fb(struct drm_plane
> *plane,
> >  	}
> >  }
> >
> > +/*
> > + * Update crtc_state->scaler_users for requested plane or crtc based on
> state.
> 
> I realize you're trying to re-use some logic here, but I find the overloaded
> function that works on either planes or crtcs a bit harder to follow.  Personally
> I'd prefer if this were just two separate functions, one focused on planes, one
> focused on CRTC's, even if it means duplicating a little bit of the logic.
> 

Yes, this is written as per one of the earlier feedback comment. 
I know this is bit complex, but I find it is worth to take care of all 
Checks in one place covering both crtc and plane. 
To avoid any confusion, I have put sufficient commentary 
throughout the code. let me know if you think any further
commentary is required.

> > + *
> > + * plane (in)
> > + *     NULL - for crtc
> > + *     not NULL - for plane
> > + * force_detach (in)
> > + *     unconditionally scaler will be staged for detachment from crtc/plane
> > + * Return
> > + *     0 - scaler_usage updated successfully
> > + *    error - requested scaling cannot be supported or other error condition
> > + */
> 
> Probably want to put this in kerneldoc format.

Will update in next rev.

> 
> > +int
> > +skl_update_scaler_users(
> > +	struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state,
> > +	struct intel_plane *intel_plane, struct intel_plane_state
> > +*plane_state,
> 
> Might be a little easier to drop the intel_crtc and intel_plane parameters and just
> pull them out of the state backpointers.

You know there is a check caller is doing to get right crtc pointer in 
case of check_plane:
	crtc = crtc ? crtc : plane->crtc;

If crtc isn't passed, then similar check is required in this function.
As caller is already taking care of this, passing crtc just to avoid
another check. 

> 
> > +	int force_detach)
> > +{
> > +	int need_scaling;
> > +	int idx;
> > +	int src_w, src_h, dst_w, dst_h;
> > +	int scaler_id;
> > +	struct drm_framebuffer *fb;
> > +	struct intel_crtc_scaler_state *scaler_state;
> > +
> > +	if (!intel_crtc || !crtc_state ||
> > +		(intel_plane && intel_plane->base.type ==
> DRM_PLANE_TYPE_CURSOR))
> > +		return 0;
> > +
> > +	scaler_state = &crtc_state->scaler_state;
> > +
> > +	if (!scaler_state->num_scalers) {
> > +		DRM_DEBUG_KMS("crtc_state = %p, num_scalers = %d\n",
> crtc_state,
> > +			scaler_state->num_scalers);
> > +		return 0;
> > +	}
> > +
> > +	idx = intel_plane ? drm_plane_index(&intel_plane->base) :
> SKL_CRTC_INDEX;
> > +	fb = intel_plane ? plane_state->base.fb : NULL;
> > +
> > +	if (intel_plane) {
> > +		src_w = drm_rect_width(&plane_state->src) >> 16;
> > +		src_h = drm_rect_height(&plane_state->src) >> 16;
> > +		dst_w = drm_rect_width(&plane_state->dst);
> > +		dst_h = drm_rect_height(&plane_state->dst);
> > +		scaler_id = plane_state->scaler_id;
> > +	} else {
> > +		struct drm_display_mode *adjusted_mode =
> > +			&crtc_state->base.adjusted_mode;
> > +		src_w = crtc_state->pipe_src_w;
> > +		src_h = crtc_state->pipe_src_h;
> > +		dst_w = adjusted_mode->hdisplay;
> > +		dst_h = adjusted_mode->vdisplay;
> > +		scaler_id = scaler_state->scaler_id;
> > +	}
> > +	need_scaling = (src_w != dst_w || src_h != dst_h);
> 
> We're already calculating this in the plane check function; we should probably
> just store the result in a plane_state field at that point so we don't have to re-
> calculate it here.

I think you are referring to sprite plane check, but this is not there in primary plane
check function.

> 
> 
> > +
> > +	/*
> > +	 * if plane is being disabled or scaler is no more required or force
> > +detach
> 
> Maybe it will become more clear later in the patch series, but I'm not sure I
> understand what 'force_detach' does.  Won't scalers that we don't end up re-
> assigning automatically be cleared when we nobody makes use of them in a new
> commit?

Force detach, as name suggests, it stages force freeing of a scaler if one is 
attached to crtc or plane. This is used when disabling panel fitter which 
is called from crtc disable. 

> 
> > +	 *  - free scaler binded to this plane/crtc
> > +	 *  - in order to do this, update crtc->scaler_usage
> > +	 *
> > +	 * Here scaler state in crtc_state is set free so that
> > +	 * scaler can be assigned to other user. Actual register
> > +	 * update to free the scaler is done in plane/panel-fit programming.
> > +	 * For this purpose crtc/plane_state->scaler_id isn't reset here.
> > +	 */
> > +	if (force_detach || !need_scaling || (intel_plane &&
> > +		(!fb || !plane_state->visible))) {
> > +		if (scaler_id >= 0) {
> > +			scaler_state->scaler_users &= ~(1 << idx);
> > +			scaler_state->scalers[scaler_id].in_use = 0;
> > +
> > +			DRM_DEBUG_KMS("Staged freeing scaler id %u.%u
> from %s:%d "
> > +				"crtc_state = %p scaler_users = 0x%x\n",
> > +				intel_crtc->pipe, scaler_id, intel_plane ?
> "PLANE" : "CRTC",
> > +				intel_plane ? intel_plane->base.base.id :
> > +				intel_crtc->base.base.id, crtc_state,
> > +				scaler_state->scaler_users);
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * check for rect size:
> > +	 *     min sizes in case of scaling involved
> > +	 *     max sizes in all cases
> > +	 */
> > +	if ((need_scaling &&
> 
> need_scaling is guaranteed to be true now (otherwise we would have returned
> above), so we can drop this test.
> 
> > +		(src_w < scaler_state->min_src_w || src_h < scaler_state-
> >min_src_h ||
> > +		 dst_w < scaler_state->min_dst_w || dst_h <
> > +scaler_state->min_dst_h)) ||
> > +
> > +		 src_w > scaler_state->max_src_w || src_h > scaler_state-
> >max_src_h ||
> > +		 dst_w > scaler_state->max_dst_w || dst_h > scaler_state-
> >max_dst_h) {
> > +		DRM_DEBUG_KMS("%s:%d scaler_user index %u.%u: src %ux%u
> dst %ux%u "
> > +			"size is out of scaler range\n",
> > +			intel_plane ? "PLANE" : "CRTC",
> > +			intel_plane ? intel_plane->base.base.id : intel_crtc-
> >base.base.id,
> > +			intel_crtc->pipe, idx, src_w, src_h, dst_w, dst_h);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* check colorkey */
> > +	if (intel_plane && need_scaling &&
> > +intel_colorkey_enabled(intel_plane)) {
> 
> Can drop 'need_scaling' test again.
> 
> > +		DRM_DEBUG_KMS("PLANE:%d scaling with color key not
> allowed",
> > +			intel_plane->base.base.id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Check src format */
> > +	if (intel_plane && need_scaling) {
> 
> Ditto
> 
> > +		switch (fb->pixel_format) {
> > +		case DRM_FORMAT_RGB565:
> > +		case DRM_FORMAT_XBGR8888:
> > +		case DRM_FORMAT_XRGB8888:
> > +		case DRM_FORMAT_ABGR8888:
> > +		case DRM_FORMAT_ARGB8888:
> > +		case DRM_FORMAT_XRGB2101010:
> > +		case DRM_FORMAT_ARGB2101010:
> > +		case DRM_FORMAT_XBGR2101010:
> > +		case DRM_FORMAT_ABGR2101010:
> > +		case DRM_FORMAT_YUYV:
> > +		case DRM_FORMAT_YVYU:
> > +		case DRM_FORMAT_UYVY:
> > +		case DRM_FORMAT_VYUY:
> > +			break;
> > +		default:
> > +			DRM_DEBUG_KMS("PLANE:%d FB:%d format 0x%x not
> supported\n",
> > +				intel_plane->base.base.id, fb->base.id, fb-
> >pixel_format);
> 
> Might want to add "...while scaling" to this message to avoid confusion.
> 
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* mark this plane as a scaler user in crtc_state */
> > +	scaler_state->scaler_users |= (1 << idx);
> > +	DRM_DEBUG_KMS("%s:%d staged scaling request for %ux%u->%ux%u "
> > +		"crtc_state = %p scaler_users = 0x%x\n",
> > +		intel_plane ? "PLANE" : "CRTC",
> > +		intel_plane ? intel_plane->base.base.id : intel_crtc-
> >base.base.id,
> > +		src_w, src_h, dst_w, dst_h, crtc_state, scaler_state-
> >scaler_users);
> > +	return 0;
> > +}
> > +
> >  static int
> >  intel_check_primary_plane(struct drm_plane *plane,
> >  			  struct intel_plane_state *state) diff --git
> > a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1da5087..f5d53c9 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1138,6 +1138,9 @@ void intel_mode_from_pipe_config(struct
> drm_display_mode *mode,
> >  				 struct intel_crtc_state *pipe_config);  void
> > intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);  void
> > intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
> > +int skl_update_scaler_users(struct intel_crtc *intel_crtc,
> > +	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
> > +	struct intel_plane_state *plane_state, int force_detach);
> >
> >  /* intel_dp.c */
> >  void intel_dp_init(struct drm_device *dev, int output_reg, enum port
> > port);
> > --
> > 1.7.9.5
> 
> You should probably just squash this into the later patches where you actually
> call it (e.g., patch 18) so it's more clear how/when/where it is used.

Same point came up during review of previous version, and for now it is agreed 
to keep current split to avoid redo of the whole series. I hope you are ok with this. 

> 
> 
> Matt
> 
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795