From mboxrd@z Thu Jan 1 00:00:00 1970 From: "G, Pallavi" Subject: Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4) Date: Fri, 13 Jun 2014 04:35:38 +0000 Message-ID: <1402634809.1130.34.camel@pg3-desktop> References: <1402414093-13401-1-git-send-email-matthew.d.roper@intel.com> <1402414093-13401-7-git-send-email-matthew.d.roper@intel.com> <1402574307.1130.24.camel@pg3-desktop> <20140612145732.GN4893@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 53F7F6E01A for ; Thu, 12 Jun 2014 21:35:42 -0700 (PDT) In-Reply-To: <20140612145732.GN4893@intel.com> Content-Language: en-US Content-ID: <234DEFA753F4564CB758711086B5ECDF@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Roper, Matthew D" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Thu, 2014-06-12 at 07:57 -0700, Matt Roper wrote: > On Thu, Jun 12, 2014 at 04:47:18AM -0700, G, Pallavi wrote: > > On Tue, 2014-06-10 at 08:28 -0700, Matt Roper wrote: > ... > > > @@ -8885,8 +8857,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) > > > kfree(work); > > > } > > > > > > - intel_crtc_cursor_set(crtc, NULL, 0, 0, 0) > > > > Please help me to understand how the cursor enable/disable will > > handled in the legacy path if we remove the cursor disable from > > intel_crtc_destroy > > Good question. When the driver is shutting down the DRM core tears down > all the KMS stuff associated with the driver. One of those steps is > destroying the CRTC which, as you note, previously would take care of > turning off the cursor plane. However now that the cursor exists as its > own drm_plane, and whatever is being scanned out by the cursor is a real > drm_framebuffer that the DRM core knows about, the cursor's FB should > get destroyed by the DRM core, which will trigger the cursor disable > entrypoint on the driver. The call sequence is > > drm_framebuffer_remove() -> drm_force_plane_disable() > -> intel_cursor_plane_disable() > > > > > - > > > drm_crtc_cleanup(crtc); > > > > > > kfree(intel_crtc); > > > @@ -10942,8 +10912,6 @@ out_config: > > > } > > > > > > static const struct drm_crtc_funcs intel_crtc_funcs = { > > > - .cursor_set = intel_crtc_cursor_set, > > > - .cursor_move = intel_crtc_cursor_move, > > > > I don't find the corresponding changes in the drm layer related to the > > cursor_set and cursor_move removal. Even in the patch 3 only for the > > universal plane drm_mode_cursor_universal is called what about the > > legacy path? > > Right, the driver entrypoints remain in the drm_crtc_funcs structure > because other non-i915 drivers may not have implemented universal cursor > planes, so the DRM core will still call into their .cursor_set and > .cursor_move entrypoints when they issue a legacy cursor ioctl. We > don't want to force other driver authors to update to universal planes > until they're ready, so we want to keep the old code paths working until > everyone has updated. > > Drivers like i915 that do get updated to have universal cursor planes > will no longer receive calls into these legacy entrypoints anymore > (since everything will come into the universal entrypoint), so there's > no need for us to keep the legacy .cursor_set and .cursor_move around in > our own driver. > > > Matt > Thanks for the clarification. with this my doubts are clear. I reviewed the entire patch series with respect to the legacy support as well as the universal cursor plane support in general. I dont find any issues and design gaps. I hope Matt is working on the igt based test for the universal plane support (seen some patches related to that in the mail thread). Reviewed-by: Pallavi G