On Thu, May 21, 2015 at 07:36:44PM +0300, Ville Syrjälä wrote: > On Mon, May 18, 2015 at 04:18:44PM -0700, Chandra Konduru wrote: > > There is a mplayer video failure reported with xv. > > This is because there is a request to do both plane scaling > > and colorkey. Because skl hw doesn't support plane scaling > > and colorkey at the same time, request is failed which is expected > > behavior. > > > > To make xv operate, this patch allows colorkey continue to work > > without using scaler. Then behavior would be similar to platforms > > without plane scaler support. > > > > Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90449 > > --- > > drivers/gpu/drm/i915/intel_display.c | 14 +++++++++----- > > drivers/gpu/drm/i915/intel_sprite.c | 31 +++++++++++++++++++++---------- > > 2 files changed, 30 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 7ab75c0..fc5cef0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4505,9 +4505,10 @@ skl_update_scaler_users( > > } > > > > /* check colorkey */ > > - if (intel_plane && intel_plane->ckey.flags != I915_SET_COLORKEY_NONE) { > > - DRM_DEBUG_KMS("PLANE:%d scaling with color key not allowed", > > - intel_plane->base.base.id); > > + if (WARN_ON(intel_plane && > > + intel_plane->ckey.flags != I915_SET_COLORKEY_NONE)) { > > + DRM_DEBUG_KMS("PLANE:%d scaling %ux%u->%ux%u not allowed with colorkey", > > + intel_plane->base.base.id, src_w, src_h, dst_w, dst_h); > > return -EINVAL; > > } > > > > @@ -13049,8 +13050,11 @@ intel_check_primary_plane(struct drm_plane *plane, > > intel_atomic_get_crtc_state(state->base.state, intel_crtc) : NULL; > > > > if (INTEL_INFO(dev)->gen >= 9) { > > - min_scale = 1; > > - max_scale = skl_max_scale(intel_crtc, crtc_state); > > + /* use scaler when colorkey is not required */ > > + if (to_intel_plane(plane)->ckey.flags == I915_SET_COLORKEY_NONE) { > > + min_scale = 1; > > + max_scale = skl_max_scale(intel_crtc, crtc_state); > > + } > > can_position = true; > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index 16d0e48..9004e47 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -775,7 +775,7 @@ intel_check_sprite_plane(struct drm_plane *plane, > > struct drm_rect *dst = &state->dst; > > const struct drm_rect *clip = &state->clip; > > int hscale, vscale; > > - int max_scale, min_scale; > > + int max_scale, min_scale, can_scale; > > can_scale ought to be bool. Otherwise this looks good. > So with that bikeshed done this gets: Done. Aside, intel_check_sprite_plane is a giant function, some extraction of sub-parts and helpers (like computing can_scale and stuff) would be good. A lot of the big local variable list could e.g. be moved into intel_plane_state (like the dynamic can_scale) and then we just need to pass around the state pointer to the various subfunctions. > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Queued for -next, thanks for the patch. -Daniel > > Ps. Please use my @linux.intel.com address when cc:ing me on patches. > I try to shovel all patch traffic through that and use the @intel.com > address for other stuff. > > > int pixel_size; > > int ret; > > > > @@ -800,18 +800,29 @@ intel_check_sprite_plane(struct drm_plane *plane, > > return -EINVAL; > > } > > > > + /* setup can_scale, min_scale, max_scale */ > > + if (INTEL_INFO(dev)->gen >= 9) { > > + /* use scaler when colorkey is not required */ > > + if (intel_plane->ckey.flags == I915_SET_COLORKEY_NONE) { > > + can_scale = 1; > > + min_scale = 1; > > + max_scale = skl_max_scale(intel_crtc, crtc_state); > > + } else { > > + can_scale = 0; > > + min_scale = DRM_PLANE_HELPER_NO_SCALING; > > + max_scale = DRM_PLANE_HELPER_NO_SCALING; > > + } > > + } else { > > + can_scale = intel_plane->can_scale; > > + max_scale = intel_plane->max_downscale << 16; > > + min_scale = intel_plane->can_scale ? 1 : (1 << 16); > > + } > > + > > /* > > * FIXME the following code does a bunch of fuzzy adjustments to the > > * coordinates and sizes. We probably need some way to decide whether > > * more strict checking should be done instead. > > */ > > - max_scale = intel_plane->max_downscale << 16; > > - min_scale = intel_plane->can_scale ? 1 : (1 << 16); > > - > > - if (INTEL_INFO(dev)->gen >= 9) { > > - min_scale = 1; > > - max_scale = skl_max_scale(intel_crtc, crtc_state); > > - } > > > > drm_rect_rotate(src, fb->width << 16, fb->height << 16, > > state->base.rotation); > > @@ -882,7 +893,7 @@ intel_check_sprite_plane(struct drm_plane *plane, > > * Must keep src and dst the > > * same if we can't scale. > > */ > > - if (!intel_plane->can_scale) > > + if (!can_scale) > > crtc_w &= ~1; > > > > if (crtc_w == 0) > > @@ -894,7 +905,7 @@ intel_check_sprite_plane(struct drm_plane *plane, > > if (state->visible && (src_w != crtc_w || src_h != crtc_h)) { > > unsigned int width_bytes; > > > > - WARN_ON(!intel_plane->can_scale); > > + WARN_ON(!can_scale); > > > > /* FIXME interlacing min height is 6 */ > > > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
