drm_framebuffer_cleanup cleanup..
On Fri, Dec 9, 2011 at 18:44, Dave Airlie wrote:
> On Fri, Dec 9, 2011 at 5:40 PM, Rob Clark wrote:
>> On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter wrote:
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ ? ? struct drm_device *dev = fb->dev;
+ ? ? struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+
+ ? ? DBG("destroy: FB ID: %d (%p)", fb->base.id, fb);
+
+ ? ? drm_framebuffer_cleanup(fb);
>>>
>>> This is a bit a general mess in kms. All drivers need to call this, and
>>> for added hilarity
>>> - drm_crtc.c drm_mode_rmfb has a TODO that this is missing
>>> - nouveau/radeon only do this _after_ unref'ing the backing storage
>>> - gma500 also tries to restore the kernel console here which should be
>>> ?done in lastclose (because you might disable another userspace fb on
>>> ?another output).
>>>
>>> Can I prod you to write the patches to clean this up and move
>>> drm_framebuffer_cleanup into common code?
>>
>> fyi, I'm starting to look at this.. looks like crtc/encoder/connector
>> cleanup could also benefit from some similar treatment. ?I'll start w/
>> just fb, since that is a resource that is actually dynamically
>> created/destroyed, so seems a bit more critical..
>
> I'm not sure reversing the flow here is the right answer, this is
> mainly trying to avoid midlayering things, the driver controls when
> stuff happens and it controls init the framebuffer object it controls
> destroying it as well.
The above proposal was more in the light of eventually switching to
properly ref-counted framebuffers. The common kms code would then keep
track of the references it knows about from the current modeset
configuration and hence also responsible for dropping them (i.e. what
drm_framebuffer_cleanup currently does). The driver could then still
hold onto references it needs internally and is also still in control
of fb destruction - the driver's fb_destroy would be called when the
last reference disappears.
So I don't see an "inversion of control through stupid middle-layer"
issue here, more like "resource lifetime mess through lack of
refcounting". But I agree that we can keep the drm_framebuffer_cleanup
where it currently is until it completely disappears with proper fb
refcounting.
Cheers, Daniel
--
Daniel Vetter
daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
Re: drm_framebuffer_cleanup cleanup..
On Fri, Dec 9, 2011 at 18:44, Dave Airlie airl...@gmail.com wrote:
On Fri, Dec 9, 2011 at 5:40 PM, Rob Clark rob.cl...@linaro.org wrote:
On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter dan...@ffwll.ch wrote:
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ struct drm_device *dev = fb-dev;
+ struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+
+ DBG(destroy: FB ID: %d (%p), fb-base.id, fb);
+
+ drm_framebuffer_cleanup(fb);
This is a bit a general mess in kms. All drivers need to call this, and
for added hilarity
- drm_crtc.c drm_mode_rmfb has a TODO that this is missing
- nouveau/radeon only do this _after_ unref'ing the backing storage
- gma500 also tries to restore the kernel console here which should be
done in lastclose (because you might disable another userspace fb on
another output).
Can I prod you to write the patches to clean this up and move
drm_framebuffer_cleanup into common code?
fyi, I'm starting to look at this.. looks like crtc/encoder/connector
cleanup could also benefit from some similar treatment. I'll start w/
just fb, since that is a resource that is actually dynamically
created/destroyed, so seems a bit more critical..
I'm not sure reversing the flow here is the right answer, this is
mainly trying to avoid midlayering things, the driver controls when
stuff happens and it controls init the framebuffer object it controls
destroying it as well.
The above proposal was more in the light of eventually switching to
properly ref-counted framebuffers. The common kms code would then keep
track of the references it knows about from the current modeset
configuration and hence also responsible for dropping them (i.e. what
drm_framebuffer_cleanup currently does). The driver could then still
hold onto references it needs internally and is also still in control
of fb destruction - the driver's fb_destroy would be called when the
last reference disappears.
So I don't see an inversion of control through stupid middle-layer
issue here, more like resource lifetime mess through lack of
refcounting. But I agree that we can keep the drm_framebuffer_cleanup
where it currently is until it completely disappears with proper fb
refcounting.
Cheers, Daniel
--
Daniel Vetter
daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
drm_framebuffer_cleanup cleanup..
On Fri, Dec 9, 2011 at 5:40 PM, Rob Clark wrote:
> On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter wrote:
>>> +static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
>>> +{
>>> + ? ? struct drm_device *dev = fb->dev;
>>> + ? ? struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
>>> +
>>> + ? ? DBG("destroy: FB ID: %d (%p)", fb->base.id, fb);
>>> +
>>> + ? ? drm_framebuffer_cleanup(fb);
>>
>> This is a bit a general mess in kms. All drivers need to call this, and
>> for added hilarity
>> - drm_crtc.c drm_mode_rmfb has a TODO that this is missing
>> - nouveau/radeon only do this _after_ unref'ing the backing storage
>> - gma500 also tries to restore the kernel console here which should be
>> ?done in lastclose (because you might disable another userspace fb on
>> ?another output).
>>
>> Can I prod you to write the patches to clean this up and move
>> drm_framebuffer_cleanup into common code?
>
> fyi, I'm starting to look at this.. looks like crtc/encoder/connector
> cleanup could also benefit from some similar treatment. ?I'll start w/
> just fb, since that is a resource that is actually dynamically
> created/destroyed, so seems a bit more critical..
I'm not sure reversing the flow here is the right answer, this is
mainly trying to avoid midlayering things, the driver controls when
stuff happens and it controls init the framebuffer object it controls
destroying it as well.
Dave.
drm_framebuffer_cleanup cleanup..
On Fri, Dec 9, 2011 at 11:44 AM, Dave Airlie wrote:
> On Fri, Dec 9, 2011 at 5:40 PM, Rob Clark wrote:
>> On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter wrote:
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ ? ? struct drm_device *dev = fb->dev;
+ ? ? struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+
+ ? ? DBG("destroy: FB ID: %d (%p)", fb->base.id, fb);
+
+ ? ? drm_framebuffer_cleanup(fb);
>>>
>>> This is a bit a general mess in kms. All drivers need to call this, and
>>> for added hilarity
>>> - drm_crtc.c drm_mode_rmfb has a TODO that this is missing
>>> - nouveau/radeon only do this _after_ unref'ing the backing storage
>>> - gma500 also tries to restore the kernel console here which should be
>>> ?done in lastclose (because you might disable another userspace fb on
>>> ?another output).
>>>
>>> Can I prod you to write the patches to clean this up and move
>>> drm_framebuffer_cleanup into common code?
>>
>> fyi, I'm starting to look at this.. looks like crtc/encoder/connector
>> cleanup could also benefit from some similar treatment. ?I'll start w/
>> just fb, since that is a resource that is actually dynamically
>> created/destroyed, so seems a bit more critical..
>
> I'm not sure reversing the flow here is the right answer, this is
> mainly trying to avoid midlayering things, the driver controls when
> stuff happens and it controls init the framebuffer object it controls
> destroying it as well.
ok.. leaving the drm_xyz_cleanup() call in the driver_xyz_destroy()
fxn, rather than calling the driver destroy fxn from
drm_xyz_cleanup(), would be a much smaller change. It does give the
driver a bit more rope to hang itself, and I guess at least the
drm_framebuffer_cleanup() should happen before backing store gets
unref'd so that all the CRTCs are turned off.
BR,
-R
> Dave.
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
drm_framebuffer_cleanup cleanup..
On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter wrote:
>> +static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
>> +{
>> + ? ? struct drm_device *dev = fb->dev;
>> + ? ? struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
>> +
>> + ? ? DBG("destroy: FB ID: %d (%p)", fb->base.id, fb);
>> +
>> + ? ? drm_framebuffer_cleanup(fb);
>
> This is a bit a general mess in kms. All drivers need to call this, and
> for added hilarity
> - drm_crtc.c drm_mode_rmfb has a TODO that this is missing
> - nouveau/radeon only do this _after_ unref'ing the backing storage
> - gma500 also tries to restore the kernel console here which should be
> ?done in lastclose (because you might disable another userspace fb on
> ?another output).
>
> Can I prod you to write the patches to clean this up and move
> drm_framebuffer_cleanup into common code?
fyi, I'm starting to look at this.. looks like crtc/encoder/connector
cleanup could also benefit from some similar treatment. I'll start w/
just fb, since that is a resource that is actually dynamically
created/destroyed, so seems a bit more critical..
BR,
-R
drm_framebuffer_cleanup cleanup..
On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter dan...@ffwll.ch wrote:
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ struct drm_device *dev = fb-dev;
+ struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+
+ DBG(destroy: FB ID: %d (%p), fb-base.id, fb);
+
+ drm_framebuffer_cleanup(fb);
This is a bit a general mess in kms. All drivers need to call this, and
for added hilarity
- drm_crtc.c drm_mode_rmfb has a TODO that this is missing
- nouveau/radeon only do this _after_ unref'ing the backing storage
- gma500 also tries to restore the kernel console here which should be
done in lastclose (because you might disable another userspace fb on
another output).
Can I prod you to write the patches to clean this up and move
drm_framebuffer_cleanup into common code?
fyi, I'm starting to look at this.. looks like crtc/encoder/connector
cleanup could also benefit from some similar treatment. I'll start w/
just fb, since that is a resource that is actually dynamically
created/destroyed, so seems a bit more critical..
BR,
-R
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm_framebuffer_cleanup cleanup..
On Fri, Dec 9, 2011 at 5:40 PM, Rob Clark rob.cl...@linaro.org wrote:
On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter dan...@ffwll.ch wrote:
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ struct drm_device *dev = fb-dev;
+ struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+
+ DBG(destroy: FB ID: %d (%p), fb-base.id, fb);
+
+ drm_framebuffer_cleanup(fb);
This is a bit a general mess in kms. All drivers need to call this, and
for added hilarity
- drm_crtc.c drm_mode_rmfb has a TODO that this is missing
- nouveau/radeon only do this _after_ unref'ing the backing storage
- gma500 also tries to restore the kernel console here which should be
done in lastclose (because you might disable another userspace fb on
another output).
Can I prod you to write the patches to clean this up and move
drm_framebuffer_cleanup into common code?
fyi, I'm starting to look at this.. looks like crtc/encoder/connector
cleanup could also benefit from some similar treatment. I'll start w/
just fb, since that is a resource that is actually dynamically
created/destroyed, so seems a bit more critical..
I'm not sure reversing the flow here is the right answer, this is
mainly trying to avoid midlayering things, the driver controls when
stuff happens and it controls init the framebuffer object it controls
destroying it as well.
Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm_framebuffer_cleanup cleanup..
On Fri, Dec 9, 2011 at 11:44 AM, Dave Airlie airl...@gmail.com wrote:
On Fri, Dec 9, 2011 at 5:40 PM, Rob Clark rob.cl...@linaro.org wrote:
On Wed, Oct 19, 2011 at 8:27 AM, Daniel Vetter dan...@ffwll.ch wrote:
+static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
+{
+ struct drm_device *dev = fb-dev;
+ struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+
+ DBG(destroy: FB ID: %d (%p), fb-base.id, fb);
+
+ drm_framebuffer_cleanup(fb);
This is a bit a general mess in kms. All drivers need to call this, and
for added hilarity
- drm_crtc.c drm_mode_rmfb has a TODO that this is missing
- nouveau/radeon only do this _after_ unref'ing the backing storage
- gma500 also tries to restore the kernel console here which should be
done in lastclose (because you might disable another userspace fb on
another output).
Can I prod you to write the patches to clean this up and move
drm_framebuffer_cleanup into common code?
fyi, I'm starting to look at this.. looks like crtc/encoder/connector
cleanup could also benefit from some similar treatment. I'll start w/
just fb, since that is a resource that is actually dynamically
created/destroyed, so seems a bit more critical..
I'm not sure reversing the flow here is the right answer, this is
mainly trying to avoid midlayering things, the driver controls when
stuff happens and it controls init the framebuffer object it controls
destroying it as well.
ok.. leaving the drm_xyz_cleanup() call in the driver_xyz_destroy()
fxn, rather than calling the driver destroy fxn from
drm_xyz_cleanup(), would be a much smaller change. It does give the
driver a bit more rope to hang itself, and I guess at least the
drm_framebuffer_cleanup() should happen before backing store gets
unref'd so that all the CRTCs are turned off.
BR,
-R
Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
