Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

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 <ander.conselvan.de.oliveira@intel.com>
>>>>>
>>>>> 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 <ander.conselvan.de.oliveira@intel.com>
>>>>> ---
>>>
>>> [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