GNOME Bugzilla – Bug 797025
kmssink: Add "restore-crtc" property
Last modified: 2018-08-30 12:18:29 UTC
Created attachment 373457 [details] [review] [patch] Add restore-crtc property for kmssink This adds "restore-crtc" property using which one can restore previous crtc mode. By default it is enabled, if CRTC was already active with a valid mode and kmssink set a new mode on CRTC using force-modesetting. This helps user restore previous crtc mode and get the previous session back after running a kmssink pipeline involving a force-modesetting. For e.g. When running a kmssink pipeline on rpi using force-modesetting on tty console, it was giving a blank screen after pipeline, and now with help of restore-crtc functionality, CRTC is set with previous crtc mode previously active on tty console.
Tested on raspberry pi.
Review of attachment 373457 [details] [review]: ::: sys/kms/gstkmssink.c @@ +682,3 @@ if (pres) drmModeFreePlaneResources (pres); + if (crtc && !self->restore_crtc) We should clear crtc pointer to NULL whenever we transfer ownership to self->saved_crtc instead of adding this condition. @@ +793,3 @@ + ("Failed to restore previous CRTC mode"), (NULL)); + else + drmModeFreeCrtc ((drmModeCrtc *) self->saved_crtc); First read, this seems to leak something on error. Can you comment on how the ownership of drmModeSetCrtc works in a way that code isn't leaking any DRM object ? You should set saved_crtc to NULL here. @@ +794,3 @@ + else + drmModeFreeCrtc ((drmModeCrtc *) self->saved_crtc); + } If restore_crtc is FALSE, there is still a change, due to the API, that self->saved_crtc is set. You should free it, and then set that pointer to NULL. @@ +1794,3 @@ + * Restore previous crtc setting if new crtc mode was set forcefully. + * By default this is enabled if user set crtc with a new mode on an already active + * crtc wchich was having a valid mode. Typo, wchich -> which @@ +1799,3 @@ + g_properties[PROP_RESTORE_CRTC] = + g_param_spec_boolean ("restore-crtc", "Restore CRTC mode", + "When enabled and crtc was set with a new mode, previous crtc mode will be restored on exit", Capital CRTC please. Instead of on exit, can you document the transition ?
Created attachment 373501 [details] [review] kmssink: Add "restore-crtc" property This adds "restore-crtc" property using which one can restore previous crtc mode. By default it is enabled, if CRTC was already active with a valid mode and kmssink set a new mode on CRTC using force-modesetting. This helps user restore previous crtc mode and get the previous session back after running a kmssink pipeline involving a force-modesetting. For e.g. When running a kmssink pipeline on rpi using force-modesetting on tty console, it was giving a blank screen after pipeline, and now with help of restore-crtc functionality, CRTC is set with previous crtc mode previously active on tty console. Edited-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Just applied my own review. This shouldn't be racy anymore, and is also simpler. I also fixes the wrong usage of GST_ELEMENT_ERROR() macro, it's pointless on shutdown as very often the message will be flushed.
Attachment 373501 [details] pushed as 7d79378 - kmssink: Add "restore-crtc" property
Oh, and tested on Intel DRM.
Thanks Nicholas for the review and edit, let me know if anything pending here, I can spend some time on weekend, if you want me to run some tests or confirm drm object freeing from kernel side.
Hi Devarsh, there was indeed a leak of the saved_crtc, but in error path only, it's fixed now, it shouldn't be a problem any-more. Thanks for your work ! p.s. I've written a proposal here, https://bugzilla.gnome.org/show_bug.cgi?id=797047 . In your fullscreen-overlay initial work, there is also the aspect of forcing an that isn't yet covered, but I think I have some ideas that may not require a new property.