Re: [Intel-gfx] [PATCH 4/5] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote: Retrieve current framebuffer config info from the regs and create an fb object for the buffer the BIOS or boot loader left us. This should allow for smooth transitions to userspace apps once we finish the initial configuration construction. v2: check for non-native modes and adjust (Jesse) fixup aperture and cmap frees (Imre) use unlocked unref if init_bios fails (Jesse) fix curly brace around DSPADDR check (Imre) comment failure path for pin_and_fence (Imre) v3: fixup fixup of aperture frees (Chris) v4: update to current bits (locking pin_and_fence hack) (Jesse) v5: move fb config fetch to display code (Jesse) re-order hw state readout on initial load to suit fb inherit (Jesse) re-add pin_and_fence in fbdev code to make sure we refcount properly (Je v6: rename to plane_config (Daniel) check for valid object when initializing BIOS fb (Jesse) split from plane_config readout and other display changes (Jesse) drop use_bios_fb option (Chris) update comments (Jesse) rework fbdev_init_bios for clarity (Jesse) drop fb obj ref under lock (Chris) v7: use fb object from plane_config instead (Ville) Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Hmm, quietly steals plane_config-fb you mean. Other than bikeshedding the kzalloc(intel_fbdev) and the clarity of intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb lifetime of plane_config-fb is extremely ugly though (the theft could be made a little more obvious for instance) and still leaked upon failure? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, 26 Nov 2013 14:09:48 + Chris Wilson ch...@chris-wilson.co.uk wrote: On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote: Retrieve current framebuffer config info from the regs and create an fb object for the buffer the BIOS or boot loader left us. This should allow for smooth transitions to userspace apps once we finish the initial configuration construction. v2: check for non-native modes and adjust (Jesse) fixup aperture and cmap frees (Imre) use unlocked unref if init_bios fails (Jesse) fix curly brace around DSPADDR check (Imre) comment failure path for pin_and_fence (Imre) v3: fixup fixup of aperture frees (Chris) v4: update to current bits (locking pin_and_fence hack) (Jesse) v5: move fb config fetch to display code (Jesse) re-order hw state readout on initial load to suit fb inherit (Jesse) re-add pin_and_fence in fbdev code to make sure we refcount properly (Je v6: rename to plane_config (Daniel) check for valid object when initializing BIOS fb (Jesse) split from plane_config readout and other display changes (Jesse) drop use_bios_fb option (Chris) update comments (Jesse) rework fbdev_init_bios for clarity (Jesse) drop fb obj ref under lock (Chris) v7: use fb object from plane_config instead (Ville) Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Hmm, quietly steals plane_config-fb you mean. Other than bikeshedding the kzalloc(intel_fbdev) and the clarity of intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb lifetime of plane_config-fb is extremely ugly though (the theft could be made a little more obvious for instance) and still leaked upon failure? I free it on failure in fb_init_bios, but maybe I missed an error path? I agree on the lifetime handling, but it was either this or an ugly copy of the fb object in the bios function. And even then we'd have to find a place to free it that made sense (there is no such place today). I did comment the field in the struct at least... -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, Nov 26, 2013 at 02:09:48PM +, Chris Wilson wrote: On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote: Retrieve current framebuffer config info from the regs and create an fb object for the buffer the BIOS or boot loader left us. This should allow for smooth transitions to userspace apps once we finish the initial configuration construction. v2: check for non-native modes and adjust (Jesse) fixup aperture and cmap frees (Imre) use unlocked unref if init_bios fails (Jesse) fix curly brace around DSPADDR check (Imre) comment failure path for pin_and_fence (Imre) v3: fixup fixup of aperture frees (Chris) v4: update to current bits (locking pin_and_fence hack) (Jesse) v5: move fb config fetch to display code (Jesse) re-order hw state readout on initial load to suit fb inherit (Jesse) re-add pin_and_fence in fbdev code to make sure we refcount properly (Je v6: rename to plane_config (Daniel) check for valid object when initializing BIOS fb (Jesse) split from plane_config readout and other display changes (Jesse) drop use_bios_fb option (Chris) update comments (Jesse) rework fbdev_init_bios for clarity (Jesse) drop fb obj ref under lock (Chris) v7: use fb object from plane_config instead (Ville) Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Hmm, quietly steals plane_config-fb you mean. Other than bikeshedding the kzalloc(intel_fbdev) and the clarity of intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb lifetime of plane_config-fb is extremely ugly though (the theft could be made a little more obvious for instance) and still leaked upon failure? I think the lifetime stuff for the stolen fb is actually ok. But there's other stuff that will probably gives us some good fireworks: - intel_crtc-plane_config seems to be left hanging in the air. Imo duplicating the crtc-fb pointer isnt' really all that good. I'd much prefer when we just shovel the invented fb into the crtc-fb pointer. Of course that requires us to properly adjust the refcount. - fb-obj has a very high chance to cause utter havoc on multi-pipe configs, since the bios loves to set up shared framebuffers. I guess this is untested? For now it's probably simplest to just not bother with the 2nd/3rd pipe. - We need to clean up the fbs we've created somehow - intel_framebuffer_init will at least register the framebuffer with the drm core. But since it's a driver-private fb and since we hold a reference already it'll never disappear I think. - Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Tue, 26 Nov 2013 18:54:04 +0100 Daniel Vetter dan...@ffwll.ch wrote: On Tue, Nov 26, 2013 at 02:09:48PM +, Chris Wilson wrote: On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote: Retrieve current framebuffer config info from the regs and create an fb object for the buffer the BIOS or boot loader left us. This should allow for smooth transitions to userspace apps once we finish the initial configuration construction. v2: check for non-native modes and adjust (Jesse) fixup aperture and cmap frees (Imre) use unlocked unref if init_bios fails (Jesse) fix curly brace around DSPADDR check (Imre) comment failure path for pin_and_fence (Imre) v3: fixup fixup of aperture frees (Chris) v4: update to current bits (locking pin_and_fence hack) (Jesse) v5: move fb config fetch to display code (Jesse) re-order hw state readout on initial load to suit fb inherit (Jesse) re-add pin_and_fence in fbdev code to make sure we refcount properly (Je v6: rename to plane_config (Daniel) check for valid object when initializing BIOS fb (Jesse) split from plane_config readout and other display changes (Jesse) drop use_bios_fb option (Chris) update comments (Jesse) rework fbdev_init_bios for clarity (Jesse) drop fb obj ref under lock (Chris) v7: use fb object from plane_config instead (Ville) Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Hmm, quietly steals plane_config-fb you mean. Other than bikeshedding the kzalloc(intel_fbdev) and the clarity of intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb lifetime of plane_config-fb is extremely ugly though (the theft could be made a little more obvious for instance) and still leaked upon failure? I think the lifetime stuff for the stolen fb is actually ok. But there's other stuff that will probably gives us some good fireworks: - intel_crtc-plane_config seems to be left hanging in the air. Imo duplicating the crtc-fb pointer isnt' really all that good. I'd much prefer when we just shovel the invented fb into the crtc-fb pointer. Of course that requires us to properly adjust the refcount. plane_config is just part of intel_crtc. No hanging, the struct is just bigger. Saves me from dealing with alloc/free of it. - fb-obj has a very high chance to cause utter havoc on multi-pipe configs, since the bios loves to set up shared framebuffers. I guess this is untested? For now it's probably simplest to just not bother with the 2nd/3rd pipe. There should be no havoc. The first allocation will succeed, and following ones will fail since they overlap. The BIOS takeover code will then use the one that was allocated (or in the unlikely case of multiple fbs, pick the biggest one). Or did you see some other fail there? - We need to clean up the fbs we've created somehow - intel_framebuffer_init will at least register the framebuffer with the drm core. But since it's a driver-private fb and since we hold a reference already it'll never disappear I think. I don't think so... it should look just like a user allocated buffer from the drm core POV. But generally our fbdev allocations do tend to live forever, so in that sense you're right, but it's not different than what we have today. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
