From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ander Conselvan de Oliveira Subject: Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config Date: Mon, 20 Oct 2014 13:49:43 +0300 Message-ID: <5444E8C7.8090701@gmail.com> References: <1412782343-28836-1-git-send-email-conselvan2@gmail.com> <1412782343-28836-3-git-send-email-conselvan2@gmail.com> <54367CFB.4000908@intel.com> <20141019142857.GK26941@phenom.ffwll.local> <20141019143032.GL26941@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 13DE889DA8 for ; Mon, 20 Oct 2014 03:49:43 -0700 (PDT) In-Reply-To: <20141019143032.GL26941@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx List-Id: intel-gfx@lists.freedesktop.org On 10/19/2014 05:30 PM, Daniel Vetter wrote: > On Sun, Oct 19, 2014 at 04:28:57PM +0200, Daniel Vetter wrote: >> On Thu, Oct 09, 2014 at 03:18:03PM +0300, Ander Conselvan de Oliveira wrote: >>> On 10/09/2014 12:11 PM, Daniel Vetter wrote: >>>> On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote: >>>>> From: Ander Conselvan de Oliveira >>>>> >>>>> This shouldn't change the behavior of those functions, since they are >>>>> called after the new_config is made effective and that points to the >>>>> current config. In a follow up patch, the mode set sequence will be >>>>> changed so this is called before disabling crtcs, and in that case >>>>> those functions should work on the staged config. >>>>> >>>>> Signed-off-by: Ander Conselvan de Oliveira >>>>> --- >>> >>> [snip] >>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>> index 3a06c3d..9d8fe8d 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>> @@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type) >>>>> return false; >>>>> } >>>>> >>>>> +/** >>>>> + * Returns whether any output on the specified pipe will have the specified >>>>> + * type after a staged modeset is complete, i.e., the same as >>>>> + * intel_pipe_has_type() but looking at encoder->new_crtc instead of >>>>> + * encoder->crtc. >>>>> + */ >>>>> +static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type) >>>> >>>> s/drm_crtc/intel_crtc/. In general we use the most specific type >>>> for internal functions to avoid tons of upcasting. So you might want to >>>> throw a patch on top to convert the existing has_type for consistency. >>> >>> What about the functions in drm_i915_display_funcs (find_dpll, >>> crtc_mode_set)? Are these not considered internal functions or is this >>> leftover from before this rule was in place? >>> >>> If I write that consistency patch it might be easier to just convert >>> everything. >> >> leftovers, and we've been slowly converting existing code over to using >> intel structures over the past 2 years. I certainly won't stop you to >> clean up all that stuff, but it's a bit of work. So I'm ok if you just >> have the vfunc signature use the intel_ types and then immediately convert >> to a drm_ type again to avoid code churn. >> >> Or just convert it all if you feel like it. > > But if you do that please split out that rote conversion into a separate > patch for easier reviewing. Either at the start of the series (so I can > merge it right away) or at the end (only done once everything is merged) > to avoid needless rebase pain. I went down the "covert all" route. I sent the prep patches as a separate series now. Thanks, Ander