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.
BR,
Jani.
>>
>> /** @funcs: plane control functions */
>> const struct drm_plane_funcs *funcs;
>> --
>> 2.45.2
--
Jani Nikula, Intel
