[Intel-gfx] [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Daniel Vetter]

On Fri, Apr 4, 2014 at 11:11 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
> Due to my past, I am not a person who should be reviewing these
> patches because I have a tendency to fear that a single variable
> rename will break everybody's machines at this point of the code :)

That's kinda why I want your opinion ;-)

> My problem with this patch is that now we do the same thing at 3
> different points depending on the platform:
> - For VLV, we will enable infoframes at encoder->pre_enable
> - For ILK and friends, we will enable infoframes at encoder->enable
> - For HSW+, we will still enable infoframes at encoder->modeset
>
> I'd really like to have a standard behavior here :)

Longterm we want to get rid of all the ->mode_set callbacks anyway and
move this stuff into enable hooks.

> Also, for ILK/SNB/IVB, we run encoder->enable after the very end of
> the modeset sequence, and I suspect the pipe is already running at
> that point (since the we already called intel_enable_pipe, we already
> ran intel_enable_planes, and we also called ironlake_pch_enable). For
> that case, we probably need to implement all those "wait for the exact
> VSYNC moment before touching register" restrictions that are described
> on the spec. Or we find a way to also call set_infoframes at
> pre_enable on these platforms. Did we test these patches on the ILK+
> family?

Well ilk/snb/ivb are special since hdmi is only on the pch, so I think
it matters when we throw the switch on the pch transcoder. But the
encoder->enable hook is indeed _after_ the pch enable call, so I guess
we could move it to a pre_enable hook too.

Same for hsw, it shouldn't really matter there really since it's
currently in the ->mode_set callback. I guess patches on top for those
platforms would be good? We could then also ditch the remaining vblank
waits we have I think ...

> I also remember a lot of bugs that would only be seen after
> suspend/resume, so I suggest testing it too :)
>
> The good thing is that moving register writes away from the mode_set
> callbacks is good IMHO since at some point we'll want crtc_enable to
> fully setup the HW, so runtime PM will be able to work without
> requiring a crtc_mode_set call. But you need to patch HSW+ too for
> that :P

Yeah, that's always a good plan.

> This is not an ack, nack nor a reviewed-by :) If you are still
> confident, feel free to go ahead.

I think I'll go ahead and see what happens ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch