Re: [RFC PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Aditya,

the code looks correct overall. There's one place where I think it 
fails. See below.

Am 15.02.25 um 14:43 schrieb Aditya Garg:
[...]
> DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
> @@ -484,10 +537,38 @@ static const struct drm_driver appletbdrm_drm_driver = {
> static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
> {
> 	struct drm_connector *connector = &adev->connector;
> +	struct drm_plane *primary_plane;
> +	struct drm_crtc *crtc;
> +	struct drm_encoder *encoder;
> 	struct drm_device *drm = &adev->drm;
> 	struct device *dev = adev->dev;
> 	int ret;
>
> +	primary_plane = &adev->primary_plane;
> +	ret = drm_universal_plane_init(drm, primary_plane, 0,
> +				       &appletbdrm_primary_plane_funcs,
> +				       appletbdrm_primary_plane_formats,
> +				       ARRAY_SIZE(appletbdrm_primary_plane_formats),
> +				       NULL,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret)
> +		return ret;
> +	drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
> +
> +	crtc = &adev->crtc;
> +	ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
> +					&appletbdrm_crtc_funcs, NULL);
> +	if (ret)
> +		return ret;
> +	drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
> +
> +	encoder = &adev->encoder;
> +	ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
> +			       DRM_MODE_ENCODER_DAC, NULL);
> +	if (ret)
> +		return ret;
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> 	ret = drmm_mode_config_init(drm);

Try to do drmm_mode_config_init() first. The initialization of planes, 
crtcs and encoders requires it. See [1] for how other drivers order 
these calls.

> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
> @@ -530,13 +611,13 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>
> -	ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
> -					   appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
> -					   NULL, &adev->connector);
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>
> -	drm_plane_enable_fb_damage_clips(&adev->pipe.plane);

> +	drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);

This line can be removed. You've already set the plane helpers a few 
lines above.

> +	drm_plane_enable_fb_damage_clips(&adev->primary_plane);

And this call should better be done next to the plane init. The code at 
[1] again gives you an example of the preferable order.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/tiny/bochs.c#L606

>
> 	drm_mode_config_reset(drm);
>
>
> The commit history having both old and new revisions of the driver is here:
>
> https://github.com/AdityaGarg8/apple-touchbar-drv/blob/atomic/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>
> Thanks
> Aditya

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)