Message ID | 1445352851-26796-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Oct 20, 2015 at 03:54:11PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Previously rotation was ignored and wrong stride programmed > into the plane registers resulting in a corrupt image on screen. > > v2: Do not access potentialy old plane state at flip time, > but store the rotation value at the time of queing the flip. > (Ville) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL) > Cc: Sonika Jindal <sonika.jindal@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++----------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 1fc1d240d3c1..9cb29086971a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring, > } > > static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, > + unsigned int rotation, > struct intel_unpin_work *work) > { > struct drm_device *dev = intel_crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_framebuffer *fb = intel_crtc->base.primary->fb; > const enum pipe pipe = intel_crtc->pipe; > - u32 ctl, stride; > + u32 ctl, stride, tile_height; > > ctl = I915_READ(PLANE_CTL(pipe, 0)); > ctl &= ~PLANE_CTL_TILED_MASK; > @@ -11121,9 +11122,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, > * The stride is either expressed as a multiple of 64 bytes chunks for > * linear buffers or in number of tiles for tiled buffers. > */ > - stride = fb->pitches[0] / > - intel_fb_stride_alignment(dev, fb->modifier[0], > - fb->pixel_format); > + if (intel_rotation_90_or_270(rotation)) { > + /* stride = Surface height in tiles */ > + tile_height = intel_tile_height(dev, fb->pixel_format, > + fb->modifier[0], 0); > + stride = DIV_ROUND_UP(fb->height, tile_height); > + } else { > + stride = fb->pitches[0] / > + intel_fb_stride_alignment(dev, fb->modifier[0], > + fb->pixel_format); > + } > > /* > * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on > @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) > intel_pipe_update_start(crtc); > > if (INTEL_INFO(mmio_flip->i915)->gen >= 9) > - skl_do_mmio_flip(crtc, work); > + skl_do_mmio_flip(crtc, mmio_flip->rotation, work); > else > /* use_mmio_flip() retricts MMIO flips to ilk+ */ > ilk_do_mmio_flip(crtc, work); > @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work) > > static int intel_queue_mmio_flip(struct drm_device *dev, > struct drm_crtc *crtc, > - struct drm_framebuffer *fb, > - struct drm_i915_gem_object *obj, > - struct intel_engine_cs *ring, > - uint32_t flags) > + unsigned int rotation, > + struct drm_i915_gem_object *obj) > { > struct intel_mmio_flip *mmio_flip; > > @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > mmio_flip->i915 = to_i915(dev); > mmio_flip->req = i915_gem_request_reference(obj->last_write_req); > mmio_flip->crtc = to_intel_crtc(crtc); > + mmio_flip->rotation = rotation; Could just do the crtc->primary->state->rotation thing here and avoid having to pass the rotation from the caller. But the patch looks OK anyway: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > INIT_WORK(&mmio_flip->work, intel_mmio_flip_work_func); > schedule_work(&mmio_flip->work); > @@ -11438,8 +11445,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > work->gtt_offset += intel_crtc->dspaddr_offset; > > if (mmio_flip) { > - ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring, > - page_flip_flags); > + ret = intel_queue_mmio_flip(dev, crtc, > + crtc->primary->state->rotation, > + obj); > if (ret) > goto cleanup_unpin; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 27dccf35f9bc..140f949302c7 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -493,6 +493,7 @@ struct intel_mmio_flip { > struct drm_i915_private *i915; > struct drm_i915_gem_request *req; > struct intel_crtc *crtc; > + unsigned int rotation; > }; > > struct skl_pipe_wm { > -- > 1.9.1
On 20/10/15 16:08, Ville Syrjälä wrote: > On Tue, Oct 20, 2015 at 03:54:11PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Previously rotation was ignored and wrong stride programmed >> into the plane registers resulting in a corrupt image on screen. >> >> v2: Do not access potentialy old plane state at flip time, >> but store the rotation value at the time of queing the flip. >> (Ville) >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL) >> Cc: Sonika Jindal <sonika.jindal@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++----------- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> 2 files changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 1fc1d240d3c1..9cb29086971a 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring, >> } >> >> static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, >> + unsigned int rotation, >> struct intel_unpin_work *work) >> { >> struct drm_device *dev = intel_crtc->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct drm_framebuffer *fb = intel_crtc->base.primary->fb; >> const enum pipe pipe = intel_crtc->pipe; >> - u32 ctl, stride; >> + u32 ctl, stride, tile_height; >> >> ctl = I915_READ(PLANE_CTL(pipe, 0)); >> ctl &= ~PLANE_CTL_TILED_MASK; >> @@ -11121,9 +11122,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, >> * The stride is either expressed as a multiple of 64 bytes chunks for >> * linear buffers or in number of tiles for tiled buffers. >> */ >> - stride = fb->pitches[0] / >> - intel_fb_stride_alignment(dev, fb->modifier[0], >> - fb->pixel_format); >> + if (intel_rotation_90_or_270(rotation)) { >> + /* stride = Surface height in tiles */ >> + tile_height = intel_tile_height(dev, fb->pixel_format, >> + fb->modifier[0], 0); >> + stride = DIV_ROUND_UP(fb->height, tile_height); >> + } else { >> + stride = fb->pitches[0] / >> + intel_fb_stride_alignment(dev, fb->modifier[0], >> + fb->pixel_format); >> + } >> >> /* >> * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on >> @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) >> intel_pipe_update_start(crtc); >> >> if (INTEL_INFO(mmio_flip->i915)->gen >= 9) >> - skl_do_mmio_flip(crtc, work); >> + skl_do_mmio_flip(crtc, mmio_flip->rotation, work); >> else >> /* use_mmio_flip() retricts MMIO flips to ilk+ */ >> ilk_do_mmio_flip(crtc, work); >> @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work) >> >> static int intel_queue_mmio_flip(struct drm_device *dev, >> struct drm_crtc *crtc, >> - struct drm_framebuffer *fb, >> - struct drm_i915_gem_object *obj, >> - struct intel_engine_cs *ring, >> - uint32_t flags) >> + unsigned int rotation, >> + struct drm_i915_gem_object *obj) >> { >> struct intel_mmio_flip *mmio_flip; >> >> @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, >> mmio_flip->i915 = to_i915(dev); >> mmio_flip->req = i915_gem_request_reference(obj->last_write_req); >> mmio_flip->crtc = to_intel_crtc(crtc); >> + mmio_flip->rotation = rotation; > > Could just do the crtc->primary->state->rotation thing here and avoid > having to pass the rotation from the caller. Oh that would indeed be nicer. I'll do a v3. > But the patch looks OK anyway: > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Can I carry over the r-b to v3? Regards, Tvrtko
On Tue, Oct 20, 2015 at 04:11:34PM +0100, Tvrtko Ursulin wrote: > > On 20/10/15 16:08, Ville Syrjälä wrote: > > On Tue, Oct 20, 2015 at 03:54:11PM +0100, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> Previously rotation was ignored and wrong stride programmed > >> into the plane registers resulting in a corrupt image on screen. > >> > >> v2: Do not access potentialy old plane state at flip time, > >> but store the rotation value at the time of queing the flip. > >> (Ville) > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> Testcase: igt/kms_rotation_crc/primary-rotation-90-flip-stress (SKL) > >> Cc: Sonika Jindal <sonika.jindal@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++----------- > >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> 2 files changed, 20 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 1fc1d240d3c1..9cb29086971a 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring, > >> } > >> > >> static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, > >> + unsigned int rotation, > >> struct intel_unpin_work *work) > >> { > >> struct drm_device *dev = intel_crtc->base.dev; > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> struct drm_framebuffer *fb = intel_crtc->base.primary->fb; > >> const enum pipe pipe = intel_crtc->pipe; > >> - u32 ctl, stride; > >> + u32 ctl, stride, tile_height; > >> > >> ctl = I915_READ(PLANE_CTL(pipe, 0)); > >> ctl &= ~PLANE_CTL_TILED_MASK; > >> @@ -11121,9 +11122,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, > >> * The stride is either expressed as a multiple of 64 bytes chunks for > >> * linear buffers or in number of tiles for tiled buffers. > >> */ > >> - stride = fb->pitches[0] / > >> - intel_fb_stride_alignment(dev, fb->modifier[0], > >> - fb->pixel_format); > >> + if (intel_rotation_90_or_270(rotation)) { > >> + /* stride = Surface height in tiles */ > >> + tile_height = intel_tile_height(dev, fb->pixel_format, > >> + fb->modifier[0], 0); > >> + stride = DIV_ROUND_UP(fb->height, tile_height); > >> + } else { > >> + stride = fb->pitches[0] / > >> + intel_fb_stride_alignment(dev, fb->modifier[0], > >> + fb->pixel_format); > >> + } > >> > >> /* > >> * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on > >> @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) > >> intel_pipe_update_start(crtc); > >> > >> if (INTEL_INFO(mmio_flip->i915)->gen >= 9) > >> - skl_do_mmio_flip(crtc, work); > >> + skl_do_mmio_flip(crtc, mmio_flip->rotation, work); > >> else > >> /* use_mmio_flip() retricts MMIO flips to ilk+ */ > >> ilk_do_mmio_flip(crtc, work); > >> @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work) > >> > >> static int intel_queue_mmio_flip(struct drm_device *dev, > >> struct drm_crtc *crtc, > >> - struct drm_framebuffer *fb, > >> - struct drm_i915_gem_object *obj, > >> - struct intel_engine_cs *ring, > >> - uint32_t flags) > >> + unsigned int rotation, > >> + struct drm_i915_gem_object *obj) > >> { > >> struct intel_mmio_flip *mmio_flip; > >> > >> @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > >> mmio_flip->i915 = to_i915(dev); > >> mmio_flip->req = i915_gem_request_reference(obj->last_write_req); > >> mmio_flip->crtc = to_intel_crtc(crtc); > >> + mmio_flip->rotation = rotation; > > > > Could just do the crtc->primary->state->rotation thing here and avoid > > having to pass the rotation from the caller. > > Oh that would indeed be nicer. I'll do a v3. > > > But the patch looks OK anyway: > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Can I carry over the r-b to v3? Yes.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1fc1d240d3c1..9cb29086971a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11091,13 +11091,14 @@ static bool use_mmio_flip(struct intel_engine_cs *ring, } static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, + unsigned int rotation, struct intel_unpin_work *work) { struct drm_device *dev = intel_crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_framebuffer *fb = intel_crtc->base.primary->fb; const enum pipe pipe = intel_crtc->pipe; - u32 ctl, stride; + u32 ctl, stride, tile_height; ctl = I915_READ(PLANE_CTL(pipe, 0)); ctl &= ~PLANE_CTL_TILED_MASK; @@ -11121,9 +11122,16 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc, * The stride is either expressed as a multiple of 64 bytes chunks for * linear buffers or in number of tiles for tiled buffers. */ - stride = fb->pitches[0] / - intel_fb_stride_alignment(dev, fb->modifier[0], - fb->pixel_format); + if (intel_rotation_90_or_270(rotation)) { + /* stride = Surface height in tiles */ + tile_height = intel_tile_height(dev, fb->pixel_format, + fb->modifier[0], 0); + stride = DIV_ROUND_UP(fb->height, tile_height); + } else { + stride = fb->pitches[0] / + intel_fb_stride_alignment(dev, fb->modifier[0], + fb->pixel_format); + } /* * Both PLANE_CTL and PLANE_STRIDE are not updated on vblank but on @@ -11181,7 +11189,7 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) intel_pipe_update_start(crtc); if (INTEL_INFO(mmio_flip->i915)->gen >= 9) - skl_do_mmio_flip(crtc, work); + skl_do_mmio_flip(crtc, mmio_flip->rotation, work); else /* use_mmio_flip() retricts MMIO flips to ilk+ */ ilk_do_mmio_flip(crtc, work); @@ -11208,10 +11216,8 @@ static void intel_mmio_flip_work_func(struct work_struct *work) static int intel_queue_mmio_flip(struct drm_device *dev, struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_i915_gem_object *obj, - struct intel_engine_cs *ring, - uint32_t flags) + unsigned int rotation, + struct drm_i915_gem_object *obj) { struct intel_mmio_flip *mmio_flip; @@ -11222,6 +11228,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, mmio_flip->i915 = to_i915(dev); mmio_flip->req = i915_gem_request_reference(obj->last_write_req); mmio_flip->crtc = to_intel_crtc(crtc); + mmio_flip->rotation = rotation; INIT_WORK(&mmio_flip->work, intel_mmio_flip_work_func); schedule_work(&mmio_flip->work); @@ -11438,8 +11445,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, work->gtt_offset += intel_crtc->dspaddr_offset; if (mmio_flip) { - ret = intel_queue_mmio_flip(dev, crtc, fb, obj, ring, - page_flip_flags); + ret = intel_queue_mmio_flip(dev, crtc, + crtc->primary->state->rotation, + obj); if (ret) goto cleanup_unpin; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 27dccf35f9bc..140f949302c7 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -493,6 +493,7 @@ struct intel_mmio_flip { struct drm_i915_private *i915; struct drm_i915_gem_request *req; struct intel_crtc *crtc; + unsigned int rotation; }; struct skl_pipe_wm {