On Fri, Apr 4, 2014 at 11:11 PM, Paulo Zanoni <przanoni@xxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
