From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] Primary plane rotation regressions Date: Mon, 22 Sep 2014 11:28:04 +0300 Message-ID: <20140922082804.GS12416@intel.com> References: <1411323609-2110-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id C5D446E120 for ; Mon, 22 Sep 2014 01:28:08 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1411323609-2110-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Sun, Sep 21, 2014 at 07:20:09PM +0100, Chris Wilson wrote: > If you attempt to use xrandr --rotation inverted at the moment, the > kernel disables the output when attempting to update the plane > rotation. This is because the primary plane src/dst rectangle is never > initialised and so it attempts to restore a 0x0 sized plane. > = > There is also a lack of debug trace through the new plane KMS functions, > and a lack of error propagation. > --- > drivers/gpu/drm/i915/i915_debugfs.c | 5 +++-- > drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++------ > 2 files changed, 25 insertions(+), 8 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i= 915_debugfs.c > index af0ee7b60979..6f56b19b244a 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2471,9 +2471,10 @@ static void intel_crtc_info(struct seq_file *m, st= ruct intel_crtc *intel_crtc) > struct intel_encoder *intel_encoder; > = > if (crtc->primary->fb) > - seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d\n", > + seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d, rotation %x\n", > crtc->primary->fb->base.id, crtc->x, crtc->y, > - crtc->primary->fb->width, crtc->primary->fb->height); > + crtc->primary->fb->width, crtc->primary->fb->height, > + to_intel_plane(crtc->primary)->rotation); > else > seq_puts(m, "\tprimary plane disabled\n"); > for_each_encoder_on_crtc(dev, crtc, intel_encoder) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 22fc782449ee..1774b1941bbe 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7361,6 +7361,17 @@ static void ironlake_get_plane_config(struct intel= _crtc *crtc, > crtc->base.primary->fb->width =3D ((val >> 16) & 0xfff) + 1; > crtc->base.primary->fb->height =3D ((val >> 0) & 0xfff) + 1; > = > + to_intel_plane(crtc->base.primary)->src_x =3D 0; > + to_intel_plane(crtc->base.primary)->src_y =3D 0; > + to_intel_plane(crtc->base.primary)->src_w =3D crtc->base.primary->fb->w= idth; > + to_intel_plane(crtc->base.primary)->src_h =3D crtc->base.primary->fb->h= eight; > + > + /* XXX from offset */ > + to_intel_plane(crtc->base.primary)->crtc_x =3D 0; > + to_intel_plane(crtc->base.primary)->crtc_y =3D 0; > + to_intel_plane(crtc->base.primary)->crtc_w =3D crtc->base.primary->fb->= width; > + to_intel_plane(crtc->base.primary)->crtc_h =3D crtc->base.primary->fb->= height; I'm thinking that for now these would be better placed in intel_modeset_readout_hw_state() where we read out the primary plane enabled state as well, and there you could use pipe_src_w/h. Also the src coords need <<16. .get_plane_config() should really be renamed to .get_primary_plane_fb() or something... > + > val =3D I915_READ(DSPSTRIDE(pipe)); > crtc->base.primary->fb->pitches[0] =3D val & 0xffffffc0; > = > @@ -11497,6 +11508,8 @@ intel_primary_plane_disable(struct drm_plane *pla= ne) > if (!plane->fb) > return 0; > = > + DRM_DEBUG_KMS("CRTC:%d\n", plane->crtc->base.id); > + > BUG_ON(!plane->crtc); > = > intel_crtc =3D to_intel_crtc(plane->crtc); > @@ -11559,6 +11572,8 @@ intel_commit_primary_plane(struct drm_plane *plan= e, > struct drm_rect *src =3D &state->src; > int ret; > = > + DRM_DEBUG_KMS("CRTC:%d visible?=3D%d\n", crtc->base.id, state->visible); > + > intel_crtc_wait_for_pending_flips(crtc); > = > /* > @@ -11646,6 +11661,10 @@ intel_primary_plane_setplane(struct drm_plane *p= lane, struct drm_crtc *crtc, > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > int ret; > = > + DRM_DEBUG_KMS("src=3D(%d, %d)x(%d, %d), crtc=3D(%d, %d)x(%d, %d), crtc = active?=3D%d\n", > + src_x, src_y, src_w, src_h, crtc_x, crtc_y, crtc_w, crtc_h, > + intel_crtc->active); I don't think I like having this much debug spew from plane operations. So if you want to add these things I'd prefer DRM_DEBUG(). Also the src coords are 16.16 here, and I'd prefer we use the same fake X geometry notation that's used in __setplane_internal() and drm_rect_debug_print(). > + > state.crtc =3D crtc; > state.fb =3D fb; > = > @@ -11670,12 +11689,9 @@ intel_primary_plane_setplane(struct drm_plane *p= lane, struct drm_crtc *crtc, > state.orig_dst =3D state.dst; > = > ret =3D intel_check_primary_plane(plane, &state); > - if (ret) > - return ret; > - > - intel_commit_primary_plane(plane, &state); > - > - return 0; > + if (ret =3D=3D 0) > + ret =3D intel_commit_primary_plane(plane, &state); > + return ret; Just 'return intel_commit_primary_plane()' ? Matches the sprite code which is a plus since we'll probably want to unify this stuff even more. > } > = > /* Common destruction function for both primary and cursor planes */ > -- = > 2.1.0 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC