Message ID | 1477399503-1710-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 25/10/2016 13:45, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Starting from commit b63a16f6cd89 ("drm/i915: Compute display surface > offset in the plane check hook for SKL+") we've already rotated the src > coordinates by 270 degrees by the time we check if a scaler is needed > or not, so we must not account for the rotation a second time. > Previously we did these steps in the opposite order and hence the > scaler check had to deal with rotation itself. The double rotation > handling causes us to enable a scaler pretty much every time 90/270 > degree plane rotation is requested, leading to fuzzier fonts and whatnot. > > Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: drm-intel-fixes@lists.freedesktop.org > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5a036999487b..587cd604ce92 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4671,7 +4671,7 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) > > static int > skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > - unsigned scaler_user, int *scaler_id, unsigned int rotation, > + unsigned scaler_user, int *scaler_id, > int src_w, int src_h, int dst_w, int dst_h) > { > struct intel_crtc_scaler_state *scaler_state = > @@ -4680,9 +4680,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > to_intel_crtc(crtc_state->base.crtc); > int need_scaling; > > - need_scaling = drm_rotation_90_or_270(rotation) ? > - (src_h != dst_w || src_w != dst_h): > - (src_w != dst_w || src_h != dst_h); > + /* > + * Src coordinates are already rotated by 270 degrees for > + * the 90/270 degree plane rotation cases (to match the > + * GTT mapping), hence no need to account for rotation here. > + */ > + need_scaling = src_w != dst_w || src_h != dst_h; > > /* > * if plane is being disabled or scaler is no more required or force detach > @@ -4749,7 +4752,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) > intel_crtc->pipe, SKL_CRTC_INDEX); > > return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX, > - &state->scaler_state.scaler_id, DRM_ROTATE_0, > + &state->scaler_state.scaler_id, > state->pipe_src_w, state->pipe_src_h, > adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay); > } > @@ -4783,7 +4786,6 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, > ret = skl_update_scaler(crtc_state, force_detach, > drm_plane_index(&intel_plane->base), > &plane_state->scaler_id, > - plane_state->base.rotation, > drm_rect_width(&plane_state->base.src) >> 16, > drm_rect_height(&plane_state->base.src) >> 16, > drm_rect_width(&plane_state->base.dst), > Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> After this only the DRM core revert remains. Is that one something of a workaround or actually could get in? Regards, Tvrtko
On Thu, Oct 27, 2016 at 08:46:50AM +0100, Tvrtko Ursulin wrote: > > On 25/10/2016 13:45, ville.syrjala@linux.intel.com wrote: > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >Starting from commit b63a16f6cd89 ("drm/i915: Compute display surface > >offset in the plane check hook for SKL+") we've already rotated the src > >coordinates by 270 degrees by the time we check if a scaler is needed > >or not, so we must not account for the rotation a second time. > >Previously we did these steps in the opposite order and hence the > >scaler check had to deal with rotation itself. The double rotation > >handling causes us to enable a scaler pretty much every time 90/270 > >degree plane rotation is requested, leading to fuzzier fonts and whatnot. > > > >Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Cc: drm-intel-fixes@lists.freedesktop.org > >Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+") > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >--- > > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >index 5a036999487b..587cd604ce92 100644 > >--- a/drivers/gpu/drm/i915/intel_display.c > >+++ b/drivers/gpu/drm/i915/intel_display.c > >@@ -4671,7 +4671,7 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) > > > > static int > > skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > >- unsigned scaler_user, int *scaler_id, unsigned int rotation, > >+ unsigned scaler_user, int *scaler_id, > > int src_w, int src_h, int dst_w, int dst_h) > > { > > struct intel_crtc_scaler_state *scaler_state = > >@@ -4680,9 +4680,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > to_intel_crtc(crtc_state->base.crtc); > > int need_scaling; > > > >- need_scaling = drm_rotation_90_or_270(rotation) ? > >- (src_h != dst_w || src_w != dst_h): > >- (src_w != dst_w || src_h != dst_h); > >+ /* > >+ * Src coordinates are already rotated by 270 degrees for > >+ * the 90/270 degree plane rotation cases (to match the > >+ * GTT mapping), hence no need to account for rotation here. > >+ */ > >+ need_scaling = src_w != dst_w || src_h != dst_h; > > > > /* > > * if plane is being disabled or scaler is no more required or force detach > >@@ -4749,7 +4752,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) > > intel_crtc->pipe, SKL_CRTC_INDEX); > > > > return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX, > >- &state->scaler_state.scaler_id, DRM_ROTATE_0, > >+ &state->scaler_state.scaler_id, > > state->pipe_src_w, state->pipe_src_h, > > adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay); > > } > >@@ -4783,7 +4786,6 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, > > ret = skl_update_scaler(crtc_state, force_detach, > > drm_plane_index(&intel_plane->base), > > &plane_state->scaler_id, > >- plane_state->base.rotation, > > drm_rect_width(&plane_state->base.src) >> 16, > > drm_rect_height(&plane_state->base.src) >> 16, > > drm_rect_width(&plane_state->base.dst), > > > > Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > After this only the DRM core revert remains. Is that one something > of a workaround or actually could get in? It itself is a workaround. There are more reasons why it is broken for us than just getting rotation wrong (it breaks the render-vs-modeset ordering, which is both ABI and required for us to prevent GPU hangs). -Chris
On Thu, Oct 27, 2016 at 08:46:50AM +0100, Tvrtko Ursulin wrote: > > On 25/10/2016 13:45, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Starting from commit b63a16f6cd89 ("drm/i915: Compute display surface > > offset in the plane check hook for SKL+") we've already rotated the src > > coordinates by 270 degrees by the time we check if a scaler is needed > > or not, so we must not account for the rotation a second time. > > Previously we did these steps in the opposite order and hence the > > scaler check had to deal with rotation itself. The double rotation > > handling causes us to enable a scaler pretty much every time 90/270 > > degree plane rotation is requested, leading to fuzzier fonts and whatnot. > > > > Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: drm-intel-fixes@lists.freedesktop.org > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the plane check hook for SKL+") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 5a036999487b..587cd604ce92 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4671,7 +4671,7 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) > > > > static int > > skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > - unsigned scaler_user, int *scaler_id, unsigned int rotation, > > + unsigned scaler_user, int *scaler_id, > > int src_w, int src_h, int dst_w, int dst_h) > > { > > struct intel_crtc_scaler_state *scaler_state = > > @@ -4680,9 +4680,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, > > to_intel_crtc(crtc_state->base.crtc); > > int need_scaling; > > > > - need_scaling = drm_rotation_90_or_270(rotation) ? > > - (src_h != dst_w || src_w != dst_h): > > - (src_w != dst_w || src_h != dst_h); > > + /* > > + * Src coordinates are already rotated by 270 degrees for > > + * the 90/270 degree plane rotation cases (to match the > > + * GTT mapping), hence no need to account for rotation here. > > + */ > > + need_scaling = src_w != dst_w || src_h != dst_h; > > > > /* > > * if plane is being disabled or scaler is no more required or force detach > > @@ -4749,7 +4752,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) > > intel_crtc->pipe, SKL_CRTC_INDEX); > > > > return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX, > > - &state->scaler_state.scaler_id, DRM_ROTATE_0, > > + &state->scaler_state.scaler_id, > > state->pipe_src_w, state->pipe_src_h, > > adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay); > > } > > @@ -4783,7 +4786,6 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, > > ret = skl_update_scaler(crtc_state, force_detach, > > drm_plane_index(&intel_plane->base), > > &plane_state->scaler_id, > > - plane_state->base.rotation, > > drm_rect_width(&plane_state->base.src) >> 16, > > drm_rect_height(&plane_state->base.src) >> 16, > > drm_rect_width(&plane_state->base.dst), > > > > Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > After this only the DRM core revert remains. Is that one something of a > workaround or actually could get in? We could add an fb_changed boolean to drm_plane_state which drives can use to override the drm helper's decision whether to call prepare_plane. And then i915 could set that if rotation changes. That's probably the fastest quick fix (but won't type that since I'll be travelling next week). -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5a036999487b..587cd604ce92 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4671,7 +4671,7 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe) static int skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, - unsigned scaler_user, int *scaler_id, unsigned int rotation, + unsigned scaler_user, int *scaler_id, int src_w, int src_h, int dst_w, int dst_h) { struct intel_crtc_scaler_state *scaler_state = @@ -4680,9 +4680,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach, to_intel_crtc(crtc_state->base.crtc); int need_scaling; - need_scaling = drm_rotation_90_or_270(rotation) ? - (src_h != dst_w || src_w != dst_h): - (src_w != dst_w || src_h != dst_h); + /* + * Src coordinates are already rotated by 270 degrees for + * the 90/270 degree plane rotation cases (to match the + * GTT mapping), hence no need to account for rotation here. + */ + need_scaling = src_w != dst_w || src_h != dst_h; /* * if plane is being disabled or scaler is no more required or force detach @@ -4749,7 +4752,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) intel_crtc->pipe, SKL_CRTC_INDEX); return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX, - &state->scaler_state.scaler_id, DRM_ROTATE_0, + &state->scaler_state.scaler_id, state->pipe_src_w, state->pipe_src_h, adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay); } @@ -4783,7 +4786,6 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state, ret = skl_update_scaler(crtc_state, force_detach, drm_plane_index(&intel_plane->base), &plane_state->scaler_id, - plane_state->base.rotation, drm_rect_width(&plane_state->base.src) >> 16, drm_rect_height(&plane_state->base.src) >> 16, drm_rect_width(&plane_state->base.dst),