[Intel-gfx] [PATCH v2 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Mar 20 03:04:56 PDT 2015
On Fri, Mar 20, 2015 at 03:19:53PM +0530, Jindal, Sonika wrote:
>
>
> On 3/19/2015 7:58 PM, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > In preparation to movable/resizeable primary planes pass the clipped
> > plane size to .update_primary_plane().
> >
> > v2: Pass src size too and use it appropriately (Sonika)
> >
> > Cc: Sonika Jindal <sonika.jindal at intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 4 +-
> > drivers/gpu/drm/i915/intel_display.c | 81 ++++++++++++++++++++++++------------
> > 2 files changed, 57 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ec23290..054be42 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -581,7 +581,9 @@ struct drm_i915_display_funcs {
> > uint32_t flags);
> > void (*update_primary_plane)(struct drm_crtc *crtc,
> > struct drm_framebuffer *fb,
> > - int x, int y);
> > + int x, int y,
> > + int src_w, int src_h,
> > + int crtc_w, int crtc_h);
> > void (*hpd_irq_setup)(struct drm_device *dev);
> > /* clock updates for mode set */
> > /* cursor updates */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5499c8e..2e9fdaa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2482,7 +2482,9 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >
> > static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> > struct drm_framebuffer *fb,
> > - int x, int y)
> > + int x, int y,
> > + int src_w, int src_h,
> > + int crtc_w, int crtc_h)
> > {
> > struct drm_device *dev = crtc->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2514,23 +2516,22 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >
> > dspcntr |= DISPLAY_PLANE_ENABLE;
> >
> > + WARN_ONCE(src_w != crtc_w || src_h != crtc_h,
> > + "primary plane doesn't support scaling\n");
> > +
> > if (INTEL_INFO(dev)->gen < 4) {
> > if (intel_crtc->pipe == PIPE_B)
> > dspcntr |= DISPPLANE_SEL_PIPE_B;
> > -
> > - /* pipesrc and dspsize control the size that is scaled from,
> > - * which should always be the user's requested size.
> > - */
> > - I915_WRITE(DSPSIZE(plane),
> > - ((intel_crtc->config->pipe_src_h - 1) << 16) |
> > - (intel_crtc->config->pipe_src_w - 1));
> > + I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) << 16) | (crtc_w - 1));
> Shouldn't this be src_* here, like you are doing for skl? Again it
> doesn't matter because no scaling. But still it will look better if we
> use same params across.
> > I915_WRITE(DSPPOS(plane), 0);
> > } else if (IS_CHERRYVIEW(dev) && plane == PLANE_B) {
> > - I915_WRITE(PRIMSIZE(plane),
> > - ((intel_crtc->config->pipe_src_h - 1) << 16) |
> > - (intel_crtc->config->pipe_src_w - 1));
> > + I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) << 16) | (crtc_w - 1));
> Same as above.
Nope. On CHV there is a scaler on one of the planes and there we'll need
to write the dst size into PRIMSIZE, and the src size to another register.
So just using the dst size for PRIMSIZE seems like the right choice.
Also with ILK-IVB sprites the dst size is also in the plane size register,
and the source size is in the plane scale register (eg. DVSSIZE and
DVSSSCALE), so this way seems more consistent with most of the platforms.
Although if we want to expose the pixel/line doubling as fixed 2x
scaling we might need to rethink this again. But I'm going to pretend
that there's no such thing for now.
> > I915_WRITE(PRIMPOS(plane), 0);
> > I915_WRITE(PRIMCNSTALPHA(plane), 0);
> > + } else {
> > + WARN_ONCE(crtc_w != intel_crtc->config->pipe_src_w ||
> > + crtc_h != intel_crtc->config->pipe_src_h,
> > + "primary plane size doesn't match pipe size\n");
> > }
> >
> > switch (fb->pixel_format) {
> > @@ -2586,14 +2587,14 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> > if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180)) {
> > dspcntr |= DISPPLANE_ROTATE_180;
> >
> > - x += (intel_crtc->config->pipe_src_w - 1);
> > - y += (intel_crtc->config->pipe_src_h - 1);
> > + x += (src_w - 1);
> > + y += (src_h - 1);
> >
> > /* Finding the last pixel of the last line of the display
> > data and adding to linear_offset*/
> > linear_offset +=
> > - (intel_crtc->config->pipe_src_h - 1) * fb->pitches[0] +
> > - (intel_crtc->config->pipe_src_w - 1) * pixel_size;
> > + (src_h - 1) * fb->pitches[0] +
> > + (src_w - 1) * pixel_size;
> > }
> >
> > I915_WRITE(reg, dspcntr);
> > @@ -2611,7 +2612,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
> >
> > static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > struct drm_framebuffer *fb,
> > - int x, int y)
> > + int x, int y,
> > + int src_w, int src_h,
> > + int crtc_w, int crtc_h)
> > {
> > struct drm_device *dev = crtc->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2643,6 +2646,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> >
> > + WARN_ONCE(src_w != crtc_w || src_h != crtc_h,
> > + "primary plane doesn't support scaling\n");
> > +
> > + WARN_ONCE(crtc_w != intel_crtc->config->pipe_src_w ||
> > + crtc_h != intel_crtc->config->pipe_src_h,
> > + "primary plane size doesn't match pipe size\n");
> > +
> I feel this should be part of the check_plane functions where we already
> perform many size related checks.
This is just an assertion to catch anyone passing bogus values here,
and a reminder to people that if they want to extend the primary plane
capabilities there's more work to be done.
> > switch (fb->pixel_format) {
> > case DRM_FORMAT_C8:
> > dspcntr |= DISPPLANE_8BPP;
> > @@ -2686,14 +2696,14 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> > dspcntr |= DISPPLANE_ROTATE_180;
> >
> > if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> > - x += (intel_crtc->config->pipe_src_w - 1);
> > - y += (intel_crtc->config->pipe_src_h - 1);
> > + x += (src_w - 1);
> > + y += (src_h - 1);
> >
> > /* Finding the last pixel of the last line of the display
> > data and adding to linear_offset*/
> > linear_offset +=
> > - (intel_crtc->config->pipe_src_h - 1) * fb->pitches[0] +
> > - (intel_crtc->config->pipe_src_w - 1) * pixel_size;
> > + (src_h - 1) * fb->pitches[0] +
> > + (src_w - 1) * pixel_size;
> > }
> > }
> >
> > @@ -2747,7 +2757,9 @@ u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier,
> >
> > static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > struct drm_framebuffer *fb,
> > - int x, int y)
> > + int x, int y,
> > + int src_w, int src_h,
> > + int crtc_w, int crtc_h)
> > {
> > struct drm_device *dev = crtc->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -2767,6 +2779,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > PLANE_CTL_PIPE_GAMMA_ENABLE |
> > PLANE_CTL_PIPE_CSC_ENABLE;
> >
> > + WARN_ONCE(src_w != crtc_w || src_h != crtc_h,
> > + "primary plane doesn't support scaling\n");
> > +
> > switch (fb->pixel_format) {
> > case DRM_FORMAT_RGB565:
> > plane_ctl |= PLANE_CTL_FORMAT_RGB_565;
> > @@ -2826,9 +2841,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >
> > I915_WRITE(PLANE_POS(pipe, 0), 0);
> > I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> > - I915_WRITE(PLANE_SIZE(pipe, 0),
> > - (intel_crtc->config->pipe_src_h - 1) << 16 |
> > - (intel_crtc->config->pipe_src_w - 1));
> > + I915_WRITE(PLANE_SIZE(pipe, 0), ((src_h - 1) << 16) | (src_w - 1));
> > I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> > I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
> >
> > @@ -2845,12 +2858,18 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > {
> > struct drm_device *dev = crtc->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >
> > if (dev_priv->display.disable_fbc)
> > dev_priv->display.disable_fbc(dev);
> >
> > + /* FIXME: this will go badly if the fb isn't big enough */
> > to_intel_crtc(crtc)->primary_enabled = true;
> > - dev_priv->display.update_primary_plane(crtc, fb, x, y);
> > + dev_priv->display.update_primary_plane(crtc, fb, x, y,
> > + intel_crtc->config->pipe_src_w,
> > + intel_crtc->config->pipe_src_h,
> > + intel_crtc->config->pipe_src_w,
> > + intel_crtc->config->pipe_src_h);
> >
> > return 0;
> > }
> > @@ -2885,7 +2904,11 @@ static void intel_update_primary_planes(struct drm_device *dev)
> > dev_priv->display.update_primary_plane(crtc,
> > state->base.fb,
> > state->src.x1 >> 16,
> > - state->src.y1 >> 16);
> > + state->src.y1 >> 16,
> > + drm_rect_width(&state->src) >> 16,
> > + drm_rect_height(&state->src) >> 16,
> > + drm_rect_width(&state->dst),
> > + drm_rect_height(&state->dst));
> The order in intel_sprite.c is dst x, y, dest w, h, then src x, y, src
> w, h. But we can sort it later when we are actually merging both the
> update functions for primary and sprites.
Oh. Failed to notice that. But yeah easy enough to fix when we unify.
>
> Also, I think it will be better to pass crtc_x, crtc_y also to be used
> to set plane position.
That's in the next patch.
>
> -Sonika
> > }
> >
> > drm_modeset_unlock(&crtc->mutex);
> > @@ -12070,7 +12093,11 @@ intel_commit_primary_plane(struct drm_plane *plane,
> > dev_priv->display.update_primary_plane(crtc,
> > state->base.fb,
> > state->src.x1 >> 16,
> > - state->src.y1 >> 16);
> > + state->src.y1 >> 16,
> > + drm_rect_width(&state->src) >> 16,
> > + drm_rect_height(&state->src) >> 16,
> > + drm_rect_width(&state->dst),
> > + drm_rect_height(&state->dst));
> > }
> > }
> >
> >
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list