Re: [PATCH 2/2] drm/komeda: Initialize encoder possible_clones

[Raveendra Talabattula]

Hi Liviu,

Thanks for your review.

On 7/28/26 15:20, Liviu Dudau wrote:
> On Tue, Jul 21, 2026 at 02:52:27PM +0100, Raveendra Talabattula wrote:
>> Komeda leaves encoder->possible_clones unset, so it stays at 0 for every
>> encoder. When userspace asks for writeback, the DRM atomic checks
> 
> Can you tell me more about this? What are you trying to do with the writeback?

We are trying to use the Komeda writeback connector to capture the composed
CRTC output while the same CRTC is also driving the display connector.

In this configuration, both the display encoder and the writeback encoder 
are included in the CRTC encoder mask. The atomic validation then reports:

crtc96 failed valid clone check for mask 0x5

The intention is only to support the display and writeback outputs concurrently.

> 
> Please note that there is a patch series that changes the way encoders get
> created for the writeback connectors, so that can potentially affect your
> use case.
> 

Could you please point me to the patch series you are referring to?
I would like to check whether it changes how the Komeda writeback encoder
should be created or how its possible_clones mask should be initialized

>> reject the configuration because no encoder is marked as clone-compatible,
>> leading to errors such as:
>>
>> 	crtc96 failed valid clone check for mask 0x5
> 
> The error you're seeing here is due to the encoder having a non-zero "possible_clones"
> which shows there is an error in your setup earlier and nothing to do with komeda.
> 

Understood. I identified that the failure is exposed by the following upstream commit:

41b4b11da021 ("drm: Add valid clones check")

This commit added validation of the clone masks for all encoders attached to a CRTC.
Therefore, the commit is exposing an issue in the earlier encoder setup rather than
introducing a Komeda-specific failure.

I will investigate where the display and writeback encoder clone relationship
should be configured correctly. 

>>
>> Komeda does not impose per-encoder clone restrictions, [...]
> 
> Komeda doesn't care about the encoders at all as it is meant to be agnostic
> to whatever encoder is used.
> 
>> [...] so initialize
>> possible_clones for all registered encoders to the full encoder mask
>> after encoder creation. This fixes writeback validation.
>>
>> Signed-off-by: Asad Malik <asad.malik@xxxxxxx>
>> Signed-off-by: Raveendra Talabattula <raveendra.talabattula@xxxxxxx>
>> ---
>>  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 20 +++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
>> index 6ed504099188..0dcc8c05e86b 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
>> @@ -276,7 +276,10 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
>>  {
>>  	struct komeda_kms_dev *kms;
>>  	struct drm_device *drm;
>> +	struct drm_encoder *encoder;
>>  	int err;
>> +	/* Bitmap of all encoders, assigned to each encoder's possible_clones. */
>> +	u32 clone_mask = 0;
>>  
>>  	kms = devm_drm_dev_alloc(mdev->dev, &komeda_kms_driver,
>>  				 struct komeda_kms_dev, base);
>> @@ -311,6 +314,23 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
>>  
>>  	drm_mode_config_reset(drm);
>>  
>> +	/*
>> +	 * Build the full possible_clones mask once. drm_encoder_index()
>> +	 * returns the bit position assigned to each encoder and BIT() converts
>> +	 * that index into the corresponding mask value.
>> +	 *
>> +	 * Komeda does not have per-encoder clone restrictions, so every encoder
>> +	 * gets the same mask and is advertised as clone-compatible with all
>> +	 * other registered encoders.
>> +	 */
>> +	drm_for_each_encoder(encoder, drm) {
>> +		clone_mask |= BIT(drm_encoder_index(encoder));
>> +	}
>> +
>> +	drm_for_each_encoder(encoder, drm) {
>> +		encoder->possible_clones = clone_mask;
>> +	}
> 
> You're modifying all the encoders in the system here which is not the right thing.
> 

I understand the concern. My intention was to follow the approach used by:

2e012e76ad59 ("drm: mali-dp: Set encoder possible_clones")

The code only updates encoders registered with this DRM device. 
Since Komeda does not impose any per-encoder clone restrictions, 
the full encoder mask represents the intended capability and
allows the display and writeback encoders to be used concurrently.

Please let me know if there is a specific encoder in this setup 
that should not be marked as clone-compatible.

> Best regards,
> Liviu
> 
>> +
>>  	err = devm_request_irq(drm->dev, mdev->irq,
>>  			       komeda_kms_irq_handler, IRQF_SHARED,
>>  			       drm->driver->name, drm);
>> -- 
>> 2.43.0
>>
> 

Thanks,
Raveendra