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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

BR,
Jani.


>>  
>>  	/** @funcs: plane control functions */
>>  	const struct drm_plane_funcs *funcs;
>> -- 
>> 2.45.2

-- 
Jani Nikula, Intel