| Message ID | 1445514996-18733-2-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 22, 2015 at 01:56:26PM +0200, Maarten Lankhorst wrote: > Don't use plane->state directly, use the pointer from commit_plane. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 8 ++--- > drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++-------------- > 2 files changed, 45 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 4e35557bbd41..51722e657b91 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -631,16 +631,16 @@ 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, > + struct intel_plane_state *plane_state, > 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); > + uint32_t src_w, uint32_t src_h, > + struct intel_scaler *scaler); This is an ugly hack imo. Better to add the scaler pointer to the plane state, set it in compute_config once we're done and not pass it around like this. Or we also pass around a pointer to the right crtc_state. Also, if we pass around plane_state I think all the crtc_x/y/w/h and x/y and src_x/y/w/h should disappear, for a really clean vfunc interface. -Daniel > 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 2551335ada04..599d001b1415 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -178,16 +178,19 @@ 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, > +skl_update_plane(struct drm_plane *drm_plane, > + struct intel_plane_state *plane_state, > 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) > + uint32_t src_w, uint32_t src_h, > + struct intel_scaler *scaler) > { > 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_crtc *crtc = plane_state->base.crtc; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > const int pipe = intel_plane->pipe; > const int plane = intel_plane->plane + 1; > @@ -199,8 +202,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, > 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; > > plane_ctl = PLANE_CTL_ENABLE | > PLANE_CTL_PIPE_GAMMA_ENABLE | > @@ -219,8 +220,6 @@ 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); > > - scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id; > - > /* Sizes are 0 based */ > src_w--; > src_h--; > @@ -261,13 +260,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 (scaler) { > 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); > @@ -341,16 +340,18 @@ 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, > +vlv_update_plane(struct drm_plane *dplane, > + struct intel_plane_state *plane_state, > 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) > + uint32_t src_w, uint32_t src_h, > + struct intel_scaler *scaler) > { > 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; > @@ -481,16 +482,19 @@ 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, > +ivb_update_plane(struct drm_plane *plane, > + struct intel_plane_state *plane_state, > 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) > + uint32_t src_w, uint32_t src_h, > + struct intel_scaler *scaler) > { > 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_crtc *crtc = plane_state->base.crtc; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > enum pipe pipe = intel_plane->pipe; > u32 sprctl, sprscale = 0; > @@ -623,16 +627,19 @@ 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, > +ilk_update_plane(struct drm_plane *plane, > + struct intel_plane_state *plane_state, > 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) > + uint32_t src_w, uint32_t src_h, > + struct intel_scaler *scaler) > { > 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_crtc *crtc = plane_state->base.crtc; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > int pipe = intel_plane->pipe; > unsigned long dvssurf_offset, linear_offset; > @@ -932,23 +939,31 @@ static void > intel_commit_sprite_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > - 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, > + struct intel_scaler *scaler = NULL; > + > + if (state->scaler_id >= 0) { > + struct intel_crtc_state *crtc_state = > + to_intel_crtc(state->base.crtc)->config; > + > + scaler = &crtc_state->scaler_state.scalers[state->scaler_id]; > + } > + > + intel_plane->update_plane(plane, state, > 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); > + drm_rect_height(&state->src) >> 16, > + scaler); > } 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 22-10-15 om 14:58 schreef Daniel Vetter: > On Thu, Oct 22, 2015 at 01:56:26PM +0200, Maarten Lankhorst wrote: >> Don't use plane->state directly, use the pointer from commit_plane. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_drv.h | 8 ++--- >> drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++-------------- >> 2 files changed, 45 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 4e35557bbd41..51722e657b91 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -631,16 +631,16 @@ 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, >> + struct intel_plane_state *plane_state, >> 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); >> + uint32_t src_w, uint32_t src_h, >> + struct intel_scaler *scaler); > This is an ugly hack imo. Better to add the scaler pointer to the plane > state, set it in compute_config once we're done and not pass it around > like this. Or we also pass around a pointer to the right crtc_state. This won't be valid if the crtc_state is updated after the page flip, so I don't think it's a good idea to add it to the state. But we could pass the crtc_state here and nuke the rest. > Also, if we pass around plane_state I think all the crtc_x/y/w/h and x/y > and src_x/y/w/h should disappear, for a really clean vfunc interface. Agreed. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4e35557bbd41..51722e657b91 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -631,16 +631,16 @@ 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, + struct intel_plane_state *plane_state, 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); + uint32_t src_w, uint32_t src_h, + struct intel_scaler *scaler); 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 2551335ada04..599d001b1415 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -178,16 +178,19 @@ 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, +skl_update_plane(struct drm_plane *drm_plane, + struct intel_plane_state *plane_state, 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) + uint32_t src_w, uint32_t src_h, + struct intel_scaler *scaler) { 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_crtc *crtc = plane_state->base.crtc; struct drm_i915_gem_object *obj = intel_fb_obj(fb); const int pipe = intel_plane->pipe; const int plane = intel_plane->plane + 1; @@ -199,8 +202,6 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc, 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; plane_ctl = PLANE_CTL_ENABLE | PLANE_CTL_PIPE_GAMMA_ENABLE | @@ -219,8 +220,6 @@ 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); - scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id; - /* Sizes are 0 based */ src_w--; src_h--; @@ -261,13 +260,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 (scaler) { 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); @@ -341,16 +340,18 @@ 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, +vlv_update_plane(struct drm_plane *dplane, + struct intel_plane_state *plane_state, 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) + uint32_t src_w, uint32_t src_h, + struct intel_scaler *scaler) { 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; @@ -481,16 +482,19 @@ 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, +ivb_update_plane(struct drm_plane *plane, + struct intel_plane_state *plane_state, 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) + uint32_t src_w, uint32_t src_h, + struct intel_scaler *scaler) { 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_crtc *crtc = plane_state->base.crtc; struct drm_i915_gem_object *obj = intel_fb_obj(fb); enum pipe pipe = intel_plane->pipe; u32 sprctl, sprscale = 0; @@ -623,16 +627,19 @@ 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, +ilk_update_plane(struct drm_plane *plane, + struct intel_plane_state *plane_state, 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) + uint32_t src_w, uint32_t src_h, + struct intel_scaler *scaler) { 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_crtc *crtc = plane_state->base.crtc; struct drm_i915_gem_object *obj = intel_fb_obj(fb); int pipe = intel_plane->pipe; unsigned long dvssurf_offset, linear_offset; @@ -932,23 +939,31 @@ static void intel_commit_sprite_plane(struct drm_plane *plane, struct intel_plane_state *state) { - 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, + struct intel_scaler *scaler = NULL; + + if (state->scaler_id >= 0) { + struct intel_crtc_state *crtc_state = + to_intel_crtc(state->base.crtc)->config; + + scaler = &crtc_state->scaler_state.scalers[state->scaler_id]; + } + + intel_plane->update_plane(plane, state, 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); + drm_rect_height(&state->src) >> 16, + scaler); } else { - intel_plane->disable_plane(plane, crtc); + struct drm_crtc *crtc = state->base.crtc; + + intel_plane->disable_plane(plane, crtc ?: plane->crtc); } }
Don't use plane->state directly, use the pointer from commit_plane. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 8 ++--- drivers/gpu/drm/i915/intel_sprite.c | 67 +++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 30 deletions(-)