Re: [PATCH 02/16] drm/udl: Test pixel limit in mode-config's mode-valid function

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2194 bytes --]

Hi

Am 29.09.22 um 15:20 schrieb Javier Martinez Canillas:
> On 9/19/22 15:03, Thomas Zimmermann wrote:
>> The sku_pixel_limit is a per-device property, similar to the amount
>> of available video memory. Move the respective mode-valid test from
>> the connector to the mode-config structure.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +static enum drm_mode_status udl_mode_config_mode_valid(struct drm_device *dev,
>> +						       const struct drm_display_mode *mode)
>> +{
>> +	struct udl_device *udl = to_udl(dev);
>> +
>> +	if (udl->sku_pixel_limit) {
>> +		if (mode->vdisplay * mode->hdisplay > udl->sku_pixel_limit)
>> +			return MODE_MEM;
>> +	}
>> +
>> +	return MODE_OK;
>> +}
>> +
>>   static const struct drm_mode_config_funcs udl_mode_funcs = {
>>   	.fb_create = drm_gem_fb_create_with_dirty,
>> +	.mode_valid = udl_mode_config_mode_valid,
>>   	.atomic_check  = drm_atomic_helper_check,
>>   	.atomic_commit = drm_atomic_helper_commit,
>>   };
> 
> It's always confusing to me whether something has to be in the .mode_valid
> for drm_mode_config helper function or for the drm_crtc_helper_funcs. This
> driver is still using the simple-KMS at this point so that will be in the
> udl_simple_display_pipe_mode_valid() if should be the latter.
> 
> In this case since it seems to be about a pixel limit, it might make sense
> to have this constraint for the DRM mode config. But since it depends on the
> {h,v}display, I thought that needed to ask if instead should be for the CRTC.

We're testing modes against limitations of the driver or hardware. The 
rule of thumb is to use the mode_valid function of the component that 
imposes the limitation.  In the case at hand, the limit is the maximum 
number of pixels that the adapter can store. So it goes into the 
device-wide mode_valid.

Best regards
Thomas

> 
> Any in case,
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]