From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 6/6] drm/i915: Switch to unified plane cursor handling (v4) Date: Fri, 13 Jun 2014 08:46:01 +0200 Message-ID: <20140613064601.GC5821@phenom.ffwll.local> 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> <1402634809.1130.34.camel@pg3-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 9BF5C6E981 for ; Thu, 12 Jun 2014 23:46:07 -0700 (PDT) Received: by mail-wi0-f172.google.com with SMTP id hi2so291976wib.17 for ; Thu, 12 Jun 2014 23:46:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402634809.1130.34.camel@pg3-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "G, Pallavi" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Fri, Jun 13, 2014 at 04:35:38AM +0000, G, Pallavi wrote: > 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 Thanks a lot for the review and patches, all merged to dinq. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch