On Fri, Oct 25, 2024 at 12:57:50PM +0300, Jani Nikula wrote:
> On Thu, 03 Oct 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > On Wed, Oct 02, 2024 at 09:21:59PM +0300, Ville Syrjala wrote:
> >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>
> >> Hide the plane->fb/etc. footguns better by stashing them inside
> >> a "legacy" sub struct.
> >>
> >> Eventually maybe we could turn 'legacy' into a pointer
> >> that only exists on legacy drivers to completely prevent
> >> any abuse by atomic drivers...
> >
> > Hmm. I should probably make it a pointer from the start,
> > to avoid having to go through the same churn yet again.
>
> [snip]
>
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index dd718c62ac31..a2d91ee4b40c 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -663,31 +663,33 @@ struct drm_plane {
> >> /** @modifier_count: Size of the array pointed at by @modifier_count. */
> >> unsigned int modifier_count;
> >>
> >> - /**
> >> - * @crtc:
> >> - *
> >> - * Currently bound CRTC, only meaningful for non-atomic drivers. For
> >> - * atomic drivers this is forced to be NULL, atomic drivers should
> >> - * instead check &drm_plane_state.crtc.
> >> - */
> >> - struct drm_crtc *crtc;
> >> -
> >> - /**
> >> - * @fb:
> >> - *
> >> - * Currently bound framebuffer, only meaningful for non-atomic drivers.
> >> - * For atomic drivers this is forced to be NULL, atomic drivers should
> >> - * instead check &drm_plane_state.fb.
> >> - */
> >> - struct drm_framebuffer *fb;
> >> -
> >> - /**
> >> - * @old_fb:
> >> - *
> >> - * Temporary tracking of the old fb while a modeset is ongoing. Only
> >> - * used by non-atomic drivers, forced to be NULL for atomic drivers.
> >> - */
> >> - struct drm_framebuffer *old_fb;
> >> + struct {
>
> Do you mean something along the lines of:
>
> struct __plane_legacy_or_something {
>
> >> + /**
> >> + * @crtc:
> >> + *
> >> + * Currently bound CRTC, only meaningful for non-atomic drivers. For
> >> + * atomic drivers this is forced to be NULL, atomic drivers should
> >> + * instead check &drm_plane_state.crtc.
> >> + */
> >> + struct drm_crtc *crtc;
> >> +
> >> + /**
> >> + * @fb:
> >> + *
> >> + * Currently bound framebuffer, only meaningful for non-atomic drivers.
> >> + * For atomic drivers this is forced to be NULL, atomic drivers should
> >> + * instead check &drm_plane_state.fb.
> >> + */
> >> + struct drm_framebuffer *fb;
> >> +
> >> + /**
> >> + * @old_fb:
> >> + *
> >> + * Temporary tracking of the old fb while a modeset is ongoing. Only
> >> + * used by non-atomic drivers, forced to be NULL for atomic drivers.
> >> + */
> >> + struct drm_framebuffer *old_fb;
> >> + } legacy;
>
> and
>
> } __legacy;
>
> struct __plane_legacy_or_something *legacy;
>
> and initially unconditionally:
>
> p->legacy = &p->__legacy;
>
> but later, once atomic drivers have been fixed:
>
> if (!drm_core_check_feature(dev, DRIVER_COMPUTE_ATOMIC))
> p->legacy = &p->__legacy;
>
> It would make the last update really simple.
Yeah, something like that.
--
Ville Syrjälä
Intel
