On Sat, 14 Dec 2013 12:01:47 +0100 Daniel Vetter <daniel@xxxxxxxx> wrote: > But I still think the fb lifetime management is a bit broken here and we > need a few small changes: > > 1. Right here in this loop we need to assign the fb from the plane_config > ot the crtc->fb pointer and grab an fb reference for that. > > If we don't do that we'll fall over for CONFIG_FB=n > > A side-effect of that is that plane_config is now fairly redundant and we > have the problem of cleaning up the fb referenced in there somehow > (especially for CONFIG_FB=n). That's kinda the reason why I don't like it > very much ... > > The below points are for the next patch, just noting them here for the > full picture. I haven't read carefully through that patch yet, so might > all be correct already. > > 2. We need to clean up fb reference in the plane config. Iirc your current > patch 3 fails that for CONFIG_FB=n Hm yeah the ownership is less clear in the CONFIG_FB=n case. I think the driver will own the buffer, and it'll get dropped on the first mode set with a new buffer. But even then there will be no process to deref the object finally, so it'll stick around. Hm... maybe just disable it if CONFIG_FB=n is the right answer for now. > 3. fbdev needs to grab one more reference (if it decides to steal the bios > framebuffer) to make sure the fbdev survives. But besides that I don't > think we need anything else - any subsequent modeset will update > references correctly. And for the fbdev config we can rely on the fastboot > modeset paths to ellide any real updates when fbcon sets its desired > config (which hopefully matches what the bios has set up already). I thought this was correct already... I'll post with the CONFIG_FB changes and you can check again. But I take a ref in the fbdev layer on both the GEM object and the fb that we end up using, so it should have the appropriate ref in that case. > > - > > - drm_modeset_lock_all(dev); > > - drm_mode_config_reset(dev); > > - intel_modeset_setup_hw_state(dev, false); > > - drm_modeset_unlock_all(dev); > > As mention in the dpio fixup patch I'd like this code block move to be > split out in a small prep patch. Ok will do. Thanks, -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
