Re: [PATCH v3 09/11] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

Hi Thomas,

Thanks for the review

On Wed, Sep 09, 2026 at 09:24:41AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.09.26 um 16:35 schrieb Maxime Ripard:
> [...]
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 0dbf04d4aa9e..f13bb7c490c4 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -303,10 +303,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >   		break;
> >   	case DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP:
> >   		req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
> >   			     dev->mode_config.async_page_flip;
> >   		break;
> > +	case DRM_CAP_ATOMIC_RESET:
> > +		req->value = drm_atomic_can_create_state(dev);
> > +		break;
> 
> Looking at this and the other places where _can_create_state is being used,
> I'd like to present a different design.
> 
> Scratch the helper entirely and introduce a dedicated callback in
> drm_mode_config_funcs that sets up the default state.  Your current helper
> drm_atomic_commit_fill_with_defaults would be the common implementation. The
> DRM core could test for the existence of this callback to see if
> default-reset is available. Sure, we'd have to modify all drivers, but it
> would be architecturally cleaner IMHO and give full control to the drivers.

There's also an interaction with the other big series relying on
atomic_create_state: state read-out.

If you're doing state read-out, you want to if possible create a blank
state, and make the hardware fill it. If not possible, then reset the
hardware and allocate a blank state. Either way, all objects are
affected, and that's what drm_mode_config_create_state() will do there.

In the DRM_MODE_ATOMIC_RESET case, we don't want to create a blank state
for *everything* but only to what's exposed to userspace (ie, everything
but drm_private_obj). In a way, it's more akin to
drm_mode_config_reset(), but without the hardware reset part.

I still feel like two functions are easier to reason about for this, and
I don't think a helper would help: we haven't needed it so far for
drm_mode_config_reset() / drm_atomic_commit_alloc(), so it's not clear
to me what the extra modularity would bring to the table.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]