On Tue, Nov 03, 2015 at 08:31:40AM +0100, Maarten Lankhorst wrote:
> Don't use plane->state directly, use the pointer from commit_plane.
>
> Changes since v1:
> - Fix uses of plane->state->rotation and color key to use the passed state too.
> - Only pass crtc_state and plane_state to update_plane.
> Changes since v2:
> - Rebased.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 10 +--
> drivers/gpu/drm/i915/intel_sprite.c | 120 +++++++++++++++++++-----------------
> 2 files changed, 67 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d1a6071afabe..16d4627364c5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -647,16 +647,12 @@ struct intel_plane {
> /*
> * NOTE: Do not place new plane state fields here (e.g., when adding
> * new plane properties). New runtime state should now be placed in
> - * the intel_plane_state structure and accessed via drm_plane->state.
> + * the intel_plane_state structure and accessed via plane_state.
> */
>
> void (*update_plane)(struct drm_plane *plane,
> - struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - int crtc_x, int crtc_y,
> - unsigned int crtc_w, unsigned int crtc_h,
> - uint32_t x, uint32_t y,
> - uint32_t src_w, uint32_t src_h);
> + struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state);
These should be const.
> void (*disable_plane)(struct drm_plane *plane,
> struct drm_crtc *crtc);
> int (*check_plane)(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 4276c135b9f2..6d3047f9c80f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
> }
>
> static void
> -skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - int crtc_x, int crtc_y,
> - unsigned int crtc_w, unsigned int crtc_h,
> - uint32_t x, uint32_t y,
> - uint32_t src_w, uint32_t src_h)
> +skl_update_plane(struct drm_plane *drm_plane,
> + struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state)
> {
> struct drm_device *dev = drm_plane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> + struct drm_framebuffer *fb = plane_state->base.fb;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> const int pipe = intel_plane->pipe;
> const int plane = intel_plane->plane + 1;
> u32 plane_ctl, stride_div, stride;
> - const struct drm_intel_sprite_colorkey *key =
> - &to_intel_plane_state(drm_plane->state)->ckey;
> + const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> unsigned long surf_addr;
> u32 tile_height, plane_offset, plane_size;
> unsigned int rotation;
> int x_offset, y_offset;
> - struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
> - int scaler_id;
> +
What's with the extra newline?
> + int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> + uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> + uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> + uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> + uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> + uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
> + const struct intel_scaler *scaler =
> + &crtc_state->scaler_state.scalers[plane_state->scaler_id];
This looks messy. I would at least get rid of the "assign two things on
one line thing".
>
> plane_ctl = PLANE_CTL_ENABLE |
> PLANE_CTL_PIPE_GAMMA_ENABLE |
> @@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>
> - rotation = drm_plane->state->rotation;
> + rotation = plane_state->base.rotation;
> plane_ctl |= skl_plane_ctl_rotation(rotation);
>
> stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> fb->pixel_format);
>
> - scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> -
> /* Sizes are 0 based */
> src_w--;
> src_h--;
> @@ -256,13 +258,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>
> /* program plane scaler */
> - if (scaler_id >= 0) {
> + if (plane_state->scaler_id >= 0) {
> uint32_t ps_ctrl = 0;
> + int scaler_id = plane_state->scaler_id;
>
> 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;
> + ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) | scaler->mode;
> 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);
> @@ -334,24 +336,28 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> }
>
> static void
> -vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - int crtc_x, int crtc_y,
> - unsigned int crtc_w, unsigned int crtc_h,
> - uint32_t x, uint32_t y,
> - uint32_t src_w, uint32_t src_h)
> +vlv_update_plane(struct drm_plane *dplane,
> + struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state)
> {
> struct drm_device *dev = dplane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_plane *intel_plane = to_intel_plane(dplane);
> + struct drm_framebuffer *fb = plane_state->base.fb;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> int pipe = intel_plane->pipe;
> int plane = intel_plane->plane;
> u32 sprctl;
> unsigned long sprsurf_offset, linear_offset;
> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> - const struct drm_intel_sprite_colorkey *key =
> - &to_intel_plane_state(dplane->state)->ckey;
> + const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> +
> + int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> + uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> + uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> + uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> + uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> + uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>
> sprctl = SP_ENABLE;
>
> @@ -421,7 +427,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> fb->pitches[0]);
> linear_offset -= sprsurf_offset;
>
> - if (dplane->state->rotation == BIT(DRM_ROTATE_180)) {
> + if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> sprctl |= SP_ROTATE_180;
>
> x += src_w;
> @@ -474,23 +480,27 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
> }
>
> static void
> -ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - int crtc_x, int crtc_y,
> - unsigned int crtc_w, unsigned int crtc_h,
> - uint32_t x, uint32_t y,
> - uint32_t src_w, uint32_t src_h)
> +ivb_update_plane(struct drm_plane *plane,
> + struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state)
> {
> struct drm_device *dev = plane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct drm_framebuffer *fb = plane_state->base.fb;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> enum pipe pipe = intel_plane->pipe;
> u32 sprctl, sprscale = 0;
> unsigned long sprsurf_offset, linear_offset;
> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> - const struct drm_intel_sprite_colorkey *key =
> - &to_intel_plane_state(plane->state)->ckey;
> + const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> +
> + int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> + uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> + uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> + uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> + uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> + uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>
> sprctl = SPRITE_ENABLE;
>
> @@ -550,7 +560,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> pixel_size, fb->pitches[0]);
> linear_offset -= sprsurf_offset;
>
> - if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
> + if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> sprctl |= SPRITE_ROTATE_180;
>
> /* HSW and BDW does this automagically in hardware */
> @@ -612,23 +622,27 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> }
>
> static void
> -ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - int crtc_x, int crtc_y,
> - unsigned int crtc_w, unsigned int crtc_h,
> - uint32_t x, uint32_t y,
> - uint32_t src_w, uint32_t src_h)
> +ilk_update_plane(struct drm_plane *plane,
> + struct intel_crtc_state *crtc_state,
> + struct intel_plane_state *plane_state)
> {
> struct drm_device *dev = plane->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> + struct drm_framebuffer *fb = plane_state->base.fb;
> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> int pipe = intel_plane->pipe;
> unsigned long dvssurf_offset, linear_offset;
> u32 dvscntr, dvsscale;
> int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> - const struct drm_intel_sprite_colorkey *key =
> - &to_intel_plane_state(plane->state)->ckey;
> + const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> +
> + int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> + uint32_t crtc_w = drm_rect_width(&plane_state->dst);
> + uint32_t crtc_h = drm_rect_height(&plane_state->dst);
> + uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> + uint32_t src_w = drm_rect_width(&plane_state->src) >> 16;
> + uint32_t src_h = drm_rect_height(&plane_state->src) >> 16;
>
> dvscntr = DVS_ENABLE;
>
> @@ -684,7 +698,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> pixel_size, fb->pitches[0]);
> linear_offset -= dvssurf_offset;
>
> - if (plane->state->rotation == BIT(DRM_ROTATE_180)) {
> + if (plane_state->base.rotation == BIT(DRM_ROTATE_180)) {
> dvscntr |= DVS_ROTATE_180;
>
> x += src_w;
> @@ -917,23 +931,17 @@ static void
> intel_commit_sprite_plane(struct drm_plane *plane,
> struct intel_plane_state *state)
I wonder why that isn't const...
> {
> - struct drm_crtc *crtc = state->base.crtc;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> - struct drm_framebuffer *fb = state->base.fb;
> -
> - crtc = crtc ? crtc : plane->crtc;
>
> if (state->visible) {
> - intel_plane->update_plane(plane, crtc, fb,
> - state->dst.x1, state->dst.y1,
> - drm_rect_width(&state->dst),
> - drm_rect_height(&state->dst),
> - state->src.x1 >> 16,
> - state->src.y1 >> 16,
> - drm_rect_width(&state->src) >> 16,
> - drm_rect_height(&state->src) >> 16);
> + struct intel_crtc_state *crtc_state =
> + to_intel_crtc(state->base.crtc)->config;
> +
> + intel_plane->update_plane(plane, crtc_state, state);
> } else {
> - intel_plane->disable_plane(plane, crtc);
> + struct drm_crtc *crtc = state->base.crtc;
> +
> + intel_plane->disable_plane(plane, crtc ?: plane->crtc);
> }
> }
>
> --
> 2.1.0
>
> _______________________________________________
> 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
