Re: [PATCH 08/13] drm/vkms: Allow to configure multiple CRTCs

[José Expósito]

Hey Louis,

On Wed, Feb 12, 2025 at 03:12:13PM +0100, Louis Chauvet wrote:
> 
> 
> Le 11/02/2025 à 11:44, José Expósito a écrit :
> > On Thu, Jan 30, 2025 at 02:48:20PM +0100, Louis Chauvet wrote:
> > > On 29/01/25 - 12:00, José Expósito wrote:
> > > > Add a list of CRTCs to vkms_config and helper functions to add and
> > > > remove as many CRTCs as wanted.
> > > > 
> > > > For backwards compatibility, add one CRTC to the default configuration.
> > > > 
> > > > A future patch will allow to attach planes and CRTCs, but for the
> > > > moment there are no changes in the way the output is configured.
> > > > 
> > > > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> > > > Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
> > > 
> > > Co-developped-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> > > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> > > Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
> > > 
> > > [...]
> > > 
> > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > > > @@ -181,7 +181,8 @@ static int vkms_create(struct vkms_config *config)
> > > >   		goto out_devres;
> > > >   	}
> > > > -	ret = drm_vblank_init(&vkms_device->drm, 1);
> > > > +	ret = drm_vblank_init(&vkms_device->drm,
> > > > +			      vkms_config_get_num_crtcs(config));
> > > 
> > > At this point we only create one crtc, can you move this change in the
> > > commit where you create multiple crtc?
> > 
> > Since the code to add more than one CRTCs is unused but technically present, I
> > think that this is the right patch to use this function.
> 
> I don't totally agree: you can create multiple vkms_config_crtc, but the
> code only creates one drm_crtc.
> 
> For me it is more logical to keep 1 here, as the rest of the code only
> creates one CRTC. With the next patch, the code will create more than one
> CRTC, so it makes sense to use vkms_config_get_num_crtcs.
> 
> It is not a strong blocking point, but if a v3 is needed, could you change
> it?

Fair enough, moved to the next patch in my local branch.

I'll send it in v3.

Jose

> > Anyway, at the moment it'll always return 1, so it is a no-op.
> 
> The current user is only default_config, so yes I agree, it is always 1.
> 
> > I didn't change it in v2, let me know if it works for you.
> > 
> > Thanks,
> > Jose
> > 
> > > >   	if (ret) {
> > > >   		DRM_ERROR("Failed to vblank\n");
> > > >   		goto out_devres;
> > > > -- 
> > > > 2.48.1
> > > > 
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>