From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gustavo Padovan Subject: Re: [Intel-gfx] [PATCH v2 03/10] drm/i915: move checks of intel_crtc_cursor_set_obj() out Date: Tue, 23 Sep 2014 12:41:56 -0300 Message-ID: <20140923154156.GP26802@joana> References: <1411424597-31662-1-git-send-email-gustavo@padovan.org> <1411424597-31662-3-git-send-email-gustavo@padovan.org> <20140923080302.GZ12416@intel.com> 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: <20140923080302.GZ12416@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 unificat= ion > > 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/i91= 5/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_c= rtc *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 drm= _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 *pl= ane, > > 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 from > 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. > > + > > + /* 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. Gustavo