Re: [PATCH v7 8/9] drm: writeback: clean up writeback connectors in drm_connector_cleanup()

[John Harrison <John.Harrison@Igalia.com>]

On 10/22/26 04:59, Dmitry Baryshkov wrote:
> On Wed, Oct 21, 2026 at 11:22:48AM -0700, John Harrison wrote:
>> On 10/19/26 06:12, Dmitry Baryshkov wrote:
>>> Only drmm_writeback_connector_init() registers
>>> drm_writeback_connector_cleanup(). Connectors set up with
>>> drm_writeback_connector_init() (amdgpu, komeda, malidp, rcar-du, vc4)
>>> never drop the formats blob or free the jobs still on the queue, leaking
>>> them together with their framebuffer and fence references.
>>>
>>> Call drm_writeback_connector_cleanup() from drm_connector_cleanup() for
>>> writeback connectors, the same way the HDMI audio and SCDC state is torn
>>> down there, so it runs for both kinds of initialization. The drmm action
>>> now just wraps it. Clearing the blob pointer makes the second invocation
>>> a no-op and also covers drm_connector_cleanup() being called after a
>>> failed __drm_writeback_connector_init().
>>>
>>> Fixes: 935774cd71fe ("drm: Add writeback connector type")
>>> Assisted-by: LLM
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> ---
>>>    drivers/gpu/drm/drm_connector.c     |  3 +++
>>>    drivers/gpu/drm/drm_crtc_internal.h |  3 +++
>>>    drivers/gpu/drm/drm_writeback.c     | 30 +++++++++++++++++++++++-------
>>>    3 files changed, 29 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>> index 893716a9e489..1c2bef5e0636 100644
>>> --- a/drivers/gpu/drm/drm_connector.c
>>> +++ b/drivers/gpu/drm/drm_connector.c
>>> @@ -929,6 +929,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>>>    	if (connector->hdmi.scdc_work_initialized)
>>>    		cancel_delayed_work_sync(&connector->hdmi.scdc_work);
>>> +	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
>>> +		drm_writeback_connector_cleanup(connector);
>>> +
>> Could this be done via a new connector->funcs->cleanup() vfunc or something?
>> Set that inside the writeback init path and it is guaranteed to then only be
>> called for objects that explicitly requested it. That would be more robust
>> and would keep sashiko happy. It would also keep a writeback connector
>> specific declaration out of the crtc_internal header.
> I'd rather not do it. These are the generic properties of every
> writeback connector. The answer to Sahiko would be that yes, currently
> all DRM_MODE_CONNECTOR_WRITEBACK use drm_writeback_connector. Once that
> is changed for the Intel driver, the condition might/would change here.
Except for the KUNIT selftests at least. I'm not sure about the HDMI 
issues but Sashiko is correct that some of the tests hack up partial 
structures and I can reproduce a bunch of the AMD ones hitting 
out-of-bounds accesses with the 'connector_type' based clean up. Locally 
hacking it to just add an extra bool that is only set in the full init 
path fixes the issue. Using a vfunc cleanup member would be nicer than 
having a specific writeback flag ontop of the connector type. The other 
option is to re-work all the selftests to allocate full writeback 
connector objects in the first place. Again, having an explicitly set 
vfunc would fix all those and just be more robust in general.

Note, it's not just the audio selftests that go boom. E.g. 
dm_test_crtc_set_crc_source_dprx_no_connector, 
dm_test_dmub_hpd_callback_unknown_type_match and 
dm_test_register_hpd_handlers_valid all fail. I've only tried the AMD 
ones at the moment, I'm not able to run the VC4 tests.

Note further, there are a lot of unrelated failures in the AMD selftests 
at the moment :(. I had to fix a bunch of other issues to get a clean 
run! Will post some patches shortly.

Regarding i915/Xe, I have updated those patch sets to run on your new 
baseline and I have the IGTs passing. At least, as well as they did with 
the prior versions. So no particular concerns from there.

John.