[Intel-gfx] [PATCH 10/17] drm: Atomic crtc/connector updates using crtc/plane helper interfaces

[Sean Paul]

On Wed, Nov 5, 2014 at 4:44 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> I've applied all the other nits, replies to the more interesting bits
> below.
>
> On Wed, Nov 05, 2014 at 01:53:48PM -0500, Sean Paul wrote:
>> On Sun, Nov 02, 2014 at 02:19:23PM +0100, Daniel Vetter wrote:
>> > + if (new_encoder != connector_state->best_encoder) {
>>
>> nit: If you just returned early when the encoder doesn't change, you can save
>> indentation and line breaks.
>
> Hm, that would mean either a goto (I don't like that for non-exceptional
> control flow) or duplicating the debug output. I'll go with the latter and
> frob it a bit.
>

I think you can just move the debug output above the new_encoder ==
connector_state->best_encoder check.

>> > + return ret;
>> > +
>> > + has_connectors = drm_atomic_connectors_for_crtc(state,
>> > + crtc);
>> > +
>> > + if (crtc_state->enable != has_connectors) {
>>
>> This makes me a little nervous. Even though has_connectors is a bool,
>> drm_atomic_connectors_for_crtc returns an int, and this seems like something
>> that someone might "fix" in the future.
>>
>> [PATCH] drm/atomic: Use proper type for drm_atomic_connectors_for_crtc
>
> Hm, yeah. I'll rename to num_connectors and add a !!. The idea of
> returning an int and not just a bool is that drivers might care if there's
> more than one, i.e. for cloned setups. At least i915 has some special
> considerations for that in some cases.
>

Makes sense, it just jumped out at me as something that might not be
future-safe.

>
>> > + ret = drm_crtc_vblank_get(crtc);
>> > + if (ret != 0)
>> > + continue;
>> > +
>> > + old_crtc_state->enable = true;
>> > + old_crtc_state->last_vblank_count = drm_vblank_count(dev, i);
>>
>> I think you should collect last_vblank_count before drm_crtc_vblank_get
>
> vblank_get should block on anything, and I'm not sure whether the
> drm_vblank_count can't just fall over if vblank_get fails (since
> vblank_get before vblank_count is the pattern drm_irq.c uses iirc). So I'd
> prefer it this way round just for paranoia. So I think I'll stick with
> this scheme. The waiting is only done once we've grabbed all the vblank
> count, and that's the important part to ensure we don't wait for too long.
>
> This really is just all part of the "generic code has no idea about the
> dpms state of a crtc". I'll plan to fix that with the missing dpms on
> atomic pieces.

Ok, sounds reasonable. The fixup in your tree looks good, feel free to
add my R-b when you squash.

Sean


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch