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

[Javier Martinez Canillas <javierm@redhat.com>]

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.

Any in case,

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat