> -----Original Message-----
> From: Roper, Matthew D
> Sent: Wednesday, March 25, 2015 2:30 PM
> To: Konduru, Chandra
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vetter, Daniel; Conselvan De Oliveira, Ander
> Subject: Re: [PATCH 21/21 v2] drm/i915: Enable skylake sprite plane scaling
> using shared scalers
>
> On Fri, Mar 20, 2015 at 05:04:42PM -0700, Chandra Konduru wrote:
> > This patch enables skylake sprite plane display scaling using shared
> > scalers atomic desgin.
> >
> > v2:
> > -use single copy of scaler limits (Matt)
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_sprite.c | 79 +++++++++++++++++++++++++++---
> -----
> > 1 file changed, 61 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 0194390..b4452bc 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -33,6 +33,7 @@
> > #include <drm/drm_crtc.h>
> > #include <drm/drm_fourcc.h>
> > #include <drm/drm_rect.h>
> > +#include <drm/drm_atomic.h>
> > #include <drm/drm_plane_helper.h>
> > #include "intel_drv.h"
> > #include <drm/i915_drm.h>
> > @@ -191,6 +192,8 @@ skl_update_plane(struct drm_plane *drm_plane,
> struct drm_crtc *crtc,
> > const int plane = intel_plane->plane + 1;
> > u32 plane_ctl, stride_div;
> > int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > + struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
> > + int scaler_id;
> >
> > plane_ctl = I915_READ(PLANE_CTL(pipe, plane));
> >
> > @@ -274,16 +277,39 @@ skl_update_plane(struct drm_plane *drm_plane,
> struct drm_crtc *crtc,
> > stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > fb->pixel_format);
> >
> > + skl_detach_scaler(crtc, drm_plane);
> > + scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> > +
> > /* Sizes are 0 based */
> > src_w--;
> > src_h--;
> > crtc_w--;
> > crtc_h--;
> >
> > + /* program plane scaler */
> > + if (scaler_id >= 0) {
> > + uint32_t ps_ctrl = 0;
> > +
> > + 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 |
> > + crtc_state->scaler_state.scalers[scaler_id].filter;
> > + 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);
> > + I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id),
> > + ((crtc_w + 1) << 16)|(crtc_h + 1));
> > +
> > + I915_WRITE(PLANE_POS(pipe, plane), 0);
> > + I915_WRITE(PLANE_SIZE(pipe, plane), (src_h << 16) | src_w);
> > + } else {
> > + I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> > + I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> > + }
>
> These parts of the last few patches are what I was referring to earlier; I think
> rather than programming scaler registers inside the plane update (which means
> you have to keep track of who new/old owners are so that the right plane takes
> responsibility), it would be easier to just have a separate step, after planes are
> done being programmed, that writes the proper values for each scaler, based on
> the CRTC scaling state.
I got your point, but that approach has its own complexities and challenges.
I responded to your previous email regarding this one.
>
> > +
> > I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> > I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> > - I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> > - I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> > I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> > I915_WRITE(PLANE_SURF(pipe, plane), i915_gem_obj_ggtt_offset(obj));
> > POSTING_READ(PLANE_SURF(pipe, plane)); @@ -305,6 +331,8 @@
> > skl_disable_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc)
> > I915_WRITE(PLANE_CTL(pipe, plane), 0);
> > POSTING_READ(PLANE_CTL(pipe, plane));
> >
> > + skl_detach_scaler(crtc, drm_plane);
> > +
> > intel_update_sprite_watermarks(drm_plane, crtc, 0, 0, 0, false,
> > false); }
> >
> > @@ -1088,7 +1116,9 @@ static int
> > intel_check_sprite_plane(struct drm_plane *plane,
> > struct intel_plane_state *state)
> > {
> > + struct drm_device *dev = plane->dev;
> > struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
> > + struct intel_crtc_state *crtc_state;
> > struct intel_plane *intel_plane = to_intel_plane(plane);
> > struct drm_framebuffer *fb = state->base.fb;
> > int crtc_x, crtc_y;
> > @@ -1097,11 +1127,16 @@ intel_check_sprite_plane(struct drm_plane
> *plane,
> > struct drm_rect *src = &state->src;
> > struct drm_rect *dst = &state->dst;
> > const struct drm_rect *clip = &state->clip;
> > + struct intel_crtc_scaler_state *scaler_state;
> > int hscale, vscale;
> > int max_scale, min_scale;
> > int pixel_size;
> > + int ret;
> >
> > intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc);
> > + crtc_state = state->base.state ?
> > + intel_atomic_get_crtc_state(state->base.state, intel_crtc) :
> NULL;
> > + scaler_state = crtc_state ? &crtc_state->scaler_state : NULL;
> >
> > if (!fb) {
> > state->visible = false;
> > @@ -1128,6 +1163,11 @@ intel_check_sprite_plane(struct drm_plane *plane,
> > max_scale = intel_plane->max_downscale << 16;
> > min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> >
> > + if (INTEL_INFO(dev)->gen >= 9 && scaler_state && scaler_state-
> >num_scalers) {
> > + min_scale = 1;
> > + max_scale = (100 << 16) / scaler_state->min_hsr;
> > + }
> > +
> > drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > state->base.rotation);
> >
> > @@ -1223,18 +1263,18 @@ intel_check_sprite_plane(struct drm_plane
> *plane,
> > width_bytes = ((src_x * pixel_size) & 63) +
> > src_w * pixel_size;
> >
> > - if (src_w > 2048 || src_h > 2048 ||
> > - width_bytes > 4096 || fb->pitches[0] > 4096) {
> > + if (INTEL_INFO(dev)->gen < 9 && (src_w > 2048 || src_h > 2048
> ||
> > + width_bytes > 4096 || fb->pitches[0] > 4096)) {
> > DRM_DEBUG_KMS("Source dimensions exceed
> hardware limits\n");
> > return -EINVAL;
> > }
> > }
> >
> > if (state->visible) {
> > - src->x1 = src_x;
> > - src->x2 = src_x + src_w;
> > - src->y1 = src_y;
> > - src->y2 = src_y + src_h;
> > + src->x1 = src_x << 16;
> > + src->x2 = (src_x + src_w) << 16;
> > + src->y1 = src_y << 16;
> > + src->y2 = (src_y + src_h) << 16;
>
> This feels unrelated...if we're changing our i915 subclass to use 16.16 format,
> that should probably be a standalone patch, independent of your hardware
> enabling here, with an explanation of why we want to switch.
I touched this code because intel_commit_sprite_plane()
is converting to regular integer (by doing >> 16) before calling
low level update_plane. For scalers to function properly
this change is required. So I made the change as part of this series.
Code snippet from intel_commit_sprite_plane():
if (state->visible) {
crtc_x = state->dst.x1;
crtc_y = state->dst.y1;
crtc_w = drm_rect_width(&state->dst);
crtc_h = drm_rect_height(&state->dst);
src_x = state->src.x1 >> 16;
src_y = state->src.y1 >> 16;
src_w = drm_rect_width(&state->src) >> 16;
src_h = drm_rect_height(&state->src) >> 16;
intel_plane->update_plane(plane, crtc, fb, obj,
crtc_x, crtc_y, crtc_w, crtc_h,
src_x, src_y, src_w, src_h);
} else {
intel_plane->disable_plane(plane, crtc);
}
>
> > }
> >
> > dst->x1 = crtc_x;
> > @@ -1271,6 +1311,13 @@ finish:
> > intel_crtc->atomic.update_wm = true;
> > }
> >
> > + if (INTEL_INFO(dev)->gen >= 9) {
> > + ret = skl_update_scaler_users(intel_crtc, crtc_state, intel_plane,
> > + state, 0);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1301,10 +1348,10 @@ intel_commit_sprite_plane(struct drm_plane
> *plane,
> > crtc_y = state->dst.y1;
> > crtc_w = drm_rect_width(&state->dst);
> > crtc_h = drm_rect_height(&state->dst);
> > - src_x = state->src.x1;
> > - src_y = state->src.y1;
> > - src_w = drm_rect_width(&state->src);
> > - src_h = drm_rect_height(&state->src);
> > + src_x = state->src.x1 >> 16;
> > + src_y = state->src.y1 >> 16;
> > + src_w = drm_rect_width(&state->src) >> 16;
> > + src_h = drm_rect_height(&state->src) >> 16;
> > intel_plane->update_plane(plane, crtc, fb, obj,
> > crtc_x, crtc_y, crtc_w, crtc_h,
> > src_x, src_y, src_w, src_h);
> > @@ -1493,12 +1540,8 @@ intel_plane_init(struct drm_device *dev, enum
> pipe pipe, int plane)
> > }
> > break;
> > case 9:
> > - /*
> > - * FIXME: Skylake planes can be scaled (with some restrictions),
> > - * but this is for another time.
> > - */
> > - intel_plane->can_scale = false;
> > - intel_plane->max_downscale = 1;
> > + intel_plane->can_scale = true;
> > + intel_plane->max_downscale = 2; /* updated later */
> > intel_plane->update_plane = skl_update_plane;
> > intel_plane->disable_plane = skl_disable_plane;
> > intel_plane->update_colorkey = skl_update_colorkey;
> > --
> > 1.7.9.5
> >
>
> --
> 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
