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@xxxxxxxxx>
> ---
> 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.
> + *
> + * 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.
> +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.
> + 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.
> +
> + /*
> + * 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?
> + * - 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.
Matt
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
