From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/5] drm/i915: move checks of intel_crtc_cursor_set_obj() out Date: Fri, 19 Sep 2014 15:11:59 +0300 Message-ID: <20140919121159.GL12416@intel.com> References: <1411069396-28051-1-git-send-email-gustavo@padovan.org> <1411069396-28051-2-git-send-email-gustavo@padovan.org> 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: <1411069396-28051-2-git-send-email-gustavo@padovan.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Gustavo Padovan Cc: intel-gfx@lists.freedesktop.org, Gustavo Padovan , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Thu, Sep 18, 2014 at 04:43:13PM -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 unification > of plane operations such pin/unpin of fb objects on i915. > = > Signed-off-by: Gustavo Padovan > --- > drivers/gpu/drm/i915/intel_display.c | 66 ++++++++++++++++++++++++------= ------ > 1 file changed, 44 insertions(+), 22 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 1fd9b70..a68befb 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8377,7 +8377,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crt= c *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; > = > @@ -8389,30 +8389,11 @@ static int intel_crtc_cursor_set_obj(struct drm_c= rtc *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"); > - ret =3D -ENOMEM; > - goto fail; > - } > - > /* 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; > - } Hmm. I was going to say this check needs to remain here due to struct_mutex getting dropped between check() and here. But it looks like these days obj->framebuffer_references should protect us from anyone changing the tiling mode while the object is wrapped in framebuffer. So seems moving it should still work fine. > - > /* > * Global gtt pte registers are special registers which actually > * forward writes to a chunk of system memory. Which means that > @@ -8488,7 +8469,6 @@ fail_unpin: > i915_gem_object_unpin_from_display_plane(obj); > fail_locked: > mutex_unlock(&dev->struct_mutex); > -fail: > drm_gem_object_unreference_unlocked(&obj->base); That unref looks like a leftover from the days before universal planes. Should be just removed AFAICS and the stale comments about reference consumption should be removed. Please send a separate patch for this stuff. > return ret; > } > @@ -12039,16 +12019,58 @@ intel_check_cursor_plane(struct drm_plane *plan= e, > 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; > + > + crtc_w =3D drm_rect_width(&state->orig_dst); > + crtc_h =3D drm_rect_height(&state->orig_dst); Could move these a bit later since they're not needed immediately. > + > + /* 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; > + > + /* Check for which cursor types we support */ > + 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"); > + ret =3D -ENOMEM; > + goto fail; > + } > + > + /* 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); > + > +fail: > + drm_gem_object_unreference_unlocked(&obj->base); and this bug should not get copied here. > + return ret; > } > = > static int > -- = > 1.9.3 > = > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- = Ville Syrj=E4l=E4 Intel OTC