Re: [PATCH 1/2] drm: Move plane->{fb,old_fb,crtc} to legacy sub-structure

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 25, 2024 at 12:57:50PM +0300, Jani Nikula wrote:
> On Thu, 03 Oct 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Oct 02, 2024 at 09:21:59PM +0300, Ville Syrjala wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> 
> >> 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