From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out Date: Tue, 23 Sep 2014 19:08:37 +0300 Message-ID: <20140923160837.GE14241@intel.com> References: <1411424597-31662-1-git-send-email-gustavo@padovan.org> <1411424597-31662-3-git-send-email-gustavo@padovan.org> <20140923080302.GZ12416@intel.com> <20140923154156.GP26802@joana> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <20140923154156.GP26802@joana> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Gustavo Padovan Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Tue, Sep 23, 2014 at 12:41:56PM -0300, Gustavo Padovan wrote: > 2014-09-23 Ville Syrj=E4l=E4 : > = > > On Mon, Sep 22, 2014 at 07:23:10PM -0300, Gustavo Padovan wrote: > > > From: Gustavo Padovan > > > = > > > Move checks inside intel_crtc_cursor_set_obj() to > > > intel_check_cursor_plane(), we only use they there so move them out to > > > make the merge of intel_crtc_cursor_set_obj() into > > > intel_check_cursor_plane() easier. > > > = > > > This is another step toward the atomic modesetting support and unific= ation > > > of plane operations such pin/unpin of fb objects on i915. > > > = > > > v2: take Ville's comment: move crtc_{w,h} assignment a bit down in the > > > code > > > = > > > Signed-off-by: Gustavo Padovan > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 61 ++++++++++++++++++++++++--= ---------- > > > 1 file changed, 41 insertions(+), 20 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i= 915/intel_display.c > > > index 2ef1836..3f37e93 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -8373,7 +8373,7 @@ static int intel_crtc_cursor_set_obj(struct drm= _crtc *crtc, > > > struct drm_i915_private *dev_priv =3D dev->dev_private; > > > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > > enum pipe pipe =3D intel_crtc->pipe; > > > - unsigned old_width, stride; > > > + unsigned old_width; > > > uint32_t addr; > > > int ret; > > > = > > > @@ -8385,29 +8385,11 @@ static int intel_crtc_cursor_set_obj(struct d= rm_crtc *crtc, > > > goto finish; > > > } > > > = > > > - /* Check for which cursor types we support */ > > > - if (!cursor_size_ok(dev, width, height)) { > > > - DRM_DEBUG("Cursor dimension not supported\n"); > > > - return -EINVAL; > > > - } > > > - > > > - stride =3D roundup_pow_of_two(width) * 4; > > > - if (obj->base.size < stride * height) { > > > - DRM_DEBUG_KMS("buffer is too small\n"); > > > - return -ENOMEM; > > > - } > > > - > > > /* we only need to pin inside GTT if cursor is non-phy */ > > > mutex_lock(&dev->struct_mutex); > > > if (!INTEL_INFO(dev)->cursor_needs_physical) { > > > unsigned alignment; > > > = > > > - if (obj->tiling_mode) { > > > - DRM_DEBUG_KMS("cursor cannot be tiled\n"); > > > - ret =3D -EINVAL; > > > - goto fail_locked; > > > - } > > > - > > > /* > > > * Global gtt pte registers are special registers which actually > > > * forward writes to a chunk of system memory. Which means that > > > @@ -11826,16 +11808,55 @@ intel_check_cursor_plane(struct drm_plane *= plane, > > > struct intel_plane_state *state) > > > { > > > struct drm_crtc *crtc =3D state->crtc; > > > + struct drm_device *dev =3D crtc->dev; > > > struct drm_framebuffer *fb =3D state->fb; > > > struct drm_rect *dest =3D &state->dst; > > > struct drm_rect *src =3D &state->src; > > > const struct drm_rect *clip =3D &state->clip; > > > + struct drm_i915_gem_object *obj =3D intel_fb_obj(fb); > > > + int crtc_w, crtc_h; > > > + unsigned stride; > > > + int ret; > > > = > > > - return drm_plane_helper_check_update(plane, crtc, fb, > > > + ret =3D drm_plane_helper_check_update(plane, crtc, fb, > > > src, dest, clip, > > > DRM_PLANE_HELPER_NO_SCALING, > > > DRM_PLANE_HELPER_NO_SCALING, > > > true, true, &state->visible); > > > + if (ret) > > > + return ret; > > > + > > > + > > > + /* if we want to turn off the cursor ignore width and height */ > > > + if (!obj) > > > + return 0; > > > + > > > + if (fb =3D=3D crtc->cursor->fb) > > > + return 0; > > = > > Hmm. This check needs to be after the cursor/obj size checks. Otherwise > > we wouldn't reject invalid sized cursors when the fb didn't change. I > > suppose we could also just drop this check, but it can still save us fr= om > > doing the tiling check since that can't have changed if the fb hasn't > > changed so maybe it's worth keeping. But either solution is fine by me. > = > Only if this was a bug before this patch, if you look at the original > intel_commit_cursor_plane() we do two things when the fbs are the same: > = > intel_crtc_update_cursor(); > intel_frontbuffer_flip(); > = > No checks are performed, that is why I put the fbs check before any other > check to make the code as similar as possible to what it was before. I guess we already had the bug then. Not entirely unsurprising given the recent history of this code. Probably better to have the fix as a separate patch then to avoid mixing restructuring and functional changes in the same patch. > = > > > + > > > + /* Check for which cursor types we support */ > > > + crtc_w =3D drm_rect_width(&state->orig_dst); > > > + crtc_h =3D drm_rect_height(&state->orig_dst); > > > + if (!cursor_size_ok(dev, crtc_w, crtc_h)) { > > > + DRM_DEBUG("Cursor dimension not supported\n"); > > > + return -EINVAL; > > > + } > > > + > > > + stride =3D roundup_pow_of_two(crtc_w) * 4; > > > + if (obj->base.size < stride * crtc_h) { > > > + DRM_DEBUG_KMS("buffer is too small\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + /* we only need to pin inside GTT if cursor is non-phy */ > > > + mutex_lock(&dev->struct_mutex); > > > + if (!INTEL_INFO(dev)->cursor_needs_physical && obj->tiling_mode) { > > > + DRM_DEBUG_KMS("cursor cannot be tiled\n"); > > > + ret =3D -EINVAL; > > > + } > > > + mutex_unlock(&dev->struct_mutex); > > > + > > > + return ret; > > = > > 'return 0' seems better here. > = > ret can be -EINVAL, the code inside the mutex 3 lines above can set ret. Oh, somehow I missed that one. You can leave the patch as is then. > = > Gustavo -- = Ville Syrj=E4l=E4 Intel OTC