[Intel-gfx] [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe

[Paulo Zanoni]

2014-02-11 19:54 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
> On Tue, Feb 11, 2014 at 6:20 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>> 2014-02-11 15:09 GMT-02:00 Paulo Zanoni <przanoni at gmail.com>:
>>> 2014-02-11 13:44 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
>>>> On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>>>>
>>>>> 2014-02-10 15:23 GMT-02:00 Daniel Vetter <daniel at ffwll.ch>:
>>>>>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote:
>>>>>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote:
>>>>>>> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>>>>> >
>>>>>>> > We want to remove those 3 boolean arguments. This is the first step.
>>>>>>> > The "pipe" passed as the argument is always intel_crtc->pipe.
>>>>>>> >
>>>>>>> > Also adjust the function documentation.
>>>>>>> >
>>>>>>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>>>>>>
>>>>>>> Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>
>>>>>>
>>>>>> Ok, I've pulled in the entire series, but a bunch of things changed so had
>>>>>> to resolve some (minor) conflicts. Please double-check that I didn't botch
>>>>>> things up too badly.
>>>>>
>>>>> You forgot to apply patch 2, and this is probably the reason why every
>>>>> subsequent patch gave you a conflict.
>>>>
>>>> The conflicts where actually with one of Ville's patches to move the
>>>> plane enabling around. I've fixed those up but apparently then missed
>>>> the other conflict hidden underneath those.
>>>>
>>>>> You also applied patch 3 twice: once for Ironlake and once for
>>>>> Haswell. You shouldn't change the Ironlake function.
>>>>>
>>>>> Do you plan to rebase or do I need to submit patches on top?
>>>>
>>>> I've applied the missing patched and dropped the ironlake patch of the
>>>> double-merged one. So if the new tree looks ok no need to resend
>>>> anything.
>>
>> Doesn't look ok yet.
>
> Oh dear ... I've tried again. Not sure whether I should have though :(

Now it looks correct :)

> -Daniel
>
>> So previously I had "[PATCH 2/8] drm/i915: don't wait for vblank after
>> enabling pipe on HSW", which removes a wait_for_vblank on HSW.
>>
>> Then on "[PATCH 7/8] drm/i915: remove wait_for_vblank argument form
>> intel_enable_pipe" we just change the function parameters without
>> changing the function behavior.
>>
>> With this, if we bisect something to patch 2 we know the problem is
>> that we stopped waiting for a vblank, and if we bisect to patch 7 we
>> know the problem is something else.
>>
>> But since you just skipped patch 2, patch 7 is now more than just a
>> coding style change: it actually does what patch 2 was supposed to do.
>> So in a way, we can say patch 2 is not really necessary, but it was
>> written weeks before patch 7, and patch 7 should be just a result of
>> the review comments.
>>
>> And the version of "drm/i915: don't wait for vblank after enabling
>> pipe on HSW" which you just committed as a last patch instead of
>> second patch (the one that changes the argument to
>> intel_crtc_update_cursor) is just plain wrong. That needs to be
>> reverted. So either we add the original patch 2 at the right place, or
>> we completely discard it...
>>
>> I know it's common to change the patch ordering when applying to our
>> trees, but it can be quite dangerous...
>>
>> Thanks,
>> Paulo
>>
>>>>
>>>>> IMHO if a series starts getting messy to apply, I think you should
>>>>> probably just ask the author to rebase and resend the final stuff.
>>>>> Maybe with this we would be able to reduce the amount of bad merges,
>>>>> which is becoming a very common problem, at least for my patches.
>>>>
>>>> The problem is that small conflicts are really common, both because I
>>>> want people to submit against drm-intel-nightly (so that I can do the
>>>> backmerging and branch shuffling correctly) and because of our
>>>> development speed. I don't think me asking for rebases in all these
>>>> cases is the better option. I tend to poke people to double-check when
>>>> I screw things up, but I guess it's just been bad luck that recently
>>>> the conflict fallout has always hit your patches :(
>>>>
>>>> Cheers, Daniel
>>>> --
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>>
>>>
>>>
>>> --
>>> Paulo Zanoni
>>
>>
>>
>> --
>> Paulo Zanoni
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni