[Intel-gfx] [PATCH 09/20] drm/kms: Nuke dirty_info property
Daniel Vetter
daniel.vetter at ffwll.ch
Tue Aug 9 14:08:41 UTC 2016
On Tue, Aug 9, 2016 at 3:59 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
> Hmm.
>
> I don't have a strong opinion on this, but IMO the original idea of
> allowing a user-space driver to optimize away the dirty calls isn't a
> bad one.
> Since the xf86-video-vmware always assumed that all drms it has to deal
> with has this property set it's not using it.
> The modesetting driver could, otoh benefit from this if someone cared to
> implement it.
>
> What about newer compositors? All using pageflips?
Fully agree that it's a nice idea, but given that most of the drivers
with a non-nop ->dirty callback don't set generic userspace can't rely
on it at all. And specific userspace (vmware is the only one I've
found) also ignores it. ABI without users doesn't server a purpose,
hence I'd like to remove it.
Of course totally not against merging this again, if someone bothers
to fix up the bugs on the kernel side (we need consistency with
advertising it on drivers that do have a dirty callback) and
implements the corresponding userspace.
> Also at this point I'm not aware of any user of this property but the
> assumption that any user of DRM must be open-source to not be broken by
> a kernel update seems dangerous to me. I though we must remain backwards
> compatible always, not only for open-source user-space?
I routinely break the blobs running on top of i915.ko and get away
with it ;-) And imo this is just a consequence of requiring
open-source userspace for any uabi - if it breaks that open-souce
userspace then we'll fix it, no questions asked. If it breaks no
open-source user at all, you've cheated when upstreaming the interface
originally, and you get to keep all the pieces.
-Daniel
>
> Thomas
>
>
> On 08/09/2016 03:41 PM, Daniel Vetter wrote:
>> It was added way back together with the dirty_fb ioctl, but neither
>> generic xfree86-modesetting nor the vmware driver use it. Everyone is
>> supposed to just unconditionally call the dirtyfb when they do
>> frontbuffer rendering.
>>
>> And since unused uabi is bad uabi (there's reasons we require open
>> source userspace for everything) let's nuke this.
>>
>> For reference see
>>
>> commit 884840aa3ce3214259e69557be5b4ce0d781ffa4
>> Author: Jakob Bornecrantz <jakob at vmware.com>
>> Date: Thu Dec 3 23:25:47 2009 +0000
>>
>> drm: Add dirty ioctl and property
>>
>> Cc: Jakob Bornecrantz <jakob at vmware.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Sinclair Yeh <syeh at vmware.com>
>> Cc: Thomas Hellstrom <thellstrom at vmware.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> ---
>> drivers/gpu/drm/drm_crtc.c | 31 -------------------------------
>> drivers/gpu/drm/udl/udl_connector.c | 3 ---
>> drivers/gpu/drm/udl/udl_modeset.c | 2 --
>> drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 9 ---------
>> drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 11 -----------
>> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 7 -------
>> include/drm/drm_crtc.h | 7 -------
>> 7 files changed, 70 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index ad38a8a31898..deed2e2c8cf1 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -136,12 +136,6 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
>> DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>> drm_tv_subconnector_enum_list)
>>
>> -static const struct drm_prop_enum_list drm_dirty_info_enum_list[] = {
>> - { DRM_MODE_DIRTY_OFF, "Off" },
>> - { DRM_MODE_DIRTY_ON, "On" },
>> - { DRM_MODE_DIRTY_ANNOTATE, "Annotate" },
>> -};
>> -
>> struct drm_conn_prop_enum_list {
>> int type;
>> const char *name;
>> @@ -1894,31 +1888,6 @@ int drm_mode_create_aspect_ratio_property(struct drm_device *dev)
>> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property);
>>
>> /**
>> - * drm_mode_create_dirty_property - create dirty property
>> - * @dev: DRM device
>> - *
>> - * Called by a driver the first time it's needed, must be attached to desired
>> - * connectors.
>> - */
>> -int drm_mode_create_dirty_info_property(struct drm_device *dev)
>> -{
>> - struct drm_property *dirty_info;
>> -
>> - if (dev->mode_config.dirty_info_property)
>> - return 0;
>> -
>> - dirty_info =
>> - drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
>> - "dirty",
>> - drm_dirty_info_enum_list,
>> - ARRAY_SIZE(drm_dirty_info_enum_list));
>> - dev->mode_config.dirty_info_property = dirty_info;
>> -
>> - return 0;
>> -}
>> -EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
>> -
>> -/**
>> * drm_mode_create_suggested_offset_properties - create suggests offset properties
>> * @dev: DRM device
>> *
>> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
>> index 4709b54c204c..d2f57c52f7db 100644
>> --- a/drivers/gpu/drm/udl/udl_connector.c
>> +++ b/drivers/gpu/drm/udl/udl_connector.c
>> @@ -150,8 +150,5 @@ int udl_connector_init(struct drm_device *dev, struct drm_encoder *encoder)
>> drm_connector_register(connector);
>> drm_mode_connector_attach_encoder(connector, encoder);
>>
>> - drm_object_attach_property(&connector->base,
>> - dev->mode_config.dirty_info_property,
>> - 1);
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>> index f92ea9579674..73695127c573 100644
>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>> @@ -441,8 +441,6 @@ int udl_modeset_init(struct drm_device *dev)
>>
>> dev->mode_config.funcs = &udl_mode_funcs;
>>
>> - drm_mode_create_dirty_info_property(dev);
>> -
>> udl_crtc_init(dev);
>>
>> encoder = udl_encoder_init(dev);
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
>> index 63ccd9871ec9..23ec673d5e16 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
>> @@ -377,9 +377,6 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
>> drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>> drm_object_attach_property(&connector->base,
>> - dev->mode_config.dirty_info_property,
>> - 1);
>> - drm_object_attach_property(&connector->base,
>> dev_priv->hotplug_mode_update_property, 1);
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.suggested_x_property, 0);
>> @@ -421,10 +418,6 @@ int vmw_kms_ldu_init_display(struct vmw_private *dev_priv)
>> if (ret != 0)
>> goto err_free;
>>
>> - ret = drm_mode_create_dirty_info_property(dev);
>> - if (ret != 0)
>> - goto err_vblank_cleanup;
>> -
>> vmw_kms_create_implicit_placement_property(dev_priv, true);
>>
>> if (dev_priv->capabilities & SVGA_CAP_MULTIMON)
>> @@ -439,8 +432,6 @@ int vmw_kms_ldu_init_display(struct vmw_private *dev_priv)
>>
>> return 0;
>>
>> -err_vblank_cleanup:
>> - drm_vblank_cleanup(dev);
>> err_free:
>> kfree(dev_priv->ldu_priv);
>> dev_priv->ldu_priv = NULL;
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
>> index b74eae2b8594..f42359084adc 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
>> @@ -538,9 +538,6 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
>> drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>> drm_object_attach_property(&connector->base,
>> - dev->mode_config.dirty_info_property,
>> - 1);
>> - drm_object_attach_property(&connector->base,
>> dev_priv->hotplug_mode_update_property, 1);
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.suggested_x_property, 0);
>> @@ -574,10 +571,6 @@ int vmw_kms_sou_init_display(struct vmw_private *dev_priv)
>> if (unlikely(ret != 0))
>> return ret;
>>
>> - ret = drm_mode_create_dirty_info_property(dev);
>> - if (unlikely(ret != 0))
>> - goto err_vblank_cleanup;
>> -
>> vmw_kms_create_implicit_placement_property(dev_priv, false);
>>
>> for (i = 0; i < VMWGFX_NUM_DISPLAY_UNITS; ++i)
>> @@ -588,10 +581,6 @@ int vmw_kms_sou_init_display(struct vmw_private *dev_priv)
>> DRM_INFO("Screen Objects Display Unit initialized\n");
>>
>> return 0;
>> -
>> -err_vblank_cleanup:
>> - drm_vblank_cleanup(dev);
>> - return ret;
>> }
>>
>> int vmw_kms_sou_close_display(struct vmw_private *dev_priv)
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> index 41932a7c4f79..94ad8d2acf9a 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
>> @@ -1131,9 +1131,6 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
>> drm_mode_crtc_set_gamma_size(crtc, 256);
>>
>> drm_object_attach_property(&connector->base,
>> - dev->mode_config.dirty_info_property,
>> - 1);
>> - drm_object_attach_property(&connector->base,
>> dev_priv->hotplug_mode_update_property, 1);
>> drm_object_attach_property(&connector->base,
>> dev->mode_config.suggested_x_property, 0);
>> @@ -1202,10 +1199,6 @@ int vmw_kms_stdu_init_display(struct vmw_private *dev_priv)
>> if (unlikely(ret != 0))
>> return ret;
>>
>> - ret = drm_mode_create_dirty_info_property(dev);
>> - if (unlikely(ret != 0))
>> - goto err_vblank_cleanup;
>> -
>> dev_priv->active_display_unit = vmw_du_screen_target;
>>
>> vmw_kms_create_implicit_placement_property(dev_priv, false);
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index a2da9ac5f436..52edecef5325 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -2626,12 +2626,6 @@ struct drm_mode_config {
>> */
>> struct drm_property *aspect_ratio_property;
>> /**
>> - * @dirty_info_property: Optional connector property to give userspace a
>> - * hint that the DIRTY_FB ioctl should be used.
>> - */
>> - struct drm_property *dirty_info_property;
>> -
>> - /**
>> * @degamma_lut_property: Optional CRTC property to set the LUT used to
>> * convert the framebuffer's colors to linear gamma.
>> */
>> @@ -2929,7 +2923,6 @@ extern int drm_mode_create_tv_properties(struct drm_device *dev,
>> const char * const modes[]);
>> extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>> extern int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>> -extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
>> extern int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>>
>> extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list