Re: [PATCH 09/20] drm/kms: Nuke dirty_info property

[Thomas Hellstrom <thellstrom@vmware.com>]

On 08/09/2016 04:08 PM, Daniel Vetter wrote:
> On Tue, Aug 9, 2016 at 3:59 PM, Thomas Hellstrom <thellstrom@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.

So if DRM drivers with a non-nop dirty callback doesn't set this
property then I agree it should be removed and
replaced with something else once someone cares to implement user-space
code for it.

Acked-by: Thomas Hellstrom <thellstrom@vmware.com>



>
> 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@vmware.com>
>>> Date:   Thu Dec 3 23:25:47 2009 +0000
>>>
>>>     drm: Add dirty ioctl and property
>>>
>>> Cc: Jakob Bornecrantz <jakob@vmware.com>
>>> Cc: Dave Airlie <airlied@redhat.com>
>>> Cc: Sinclair Yeh <syeh@vmware.com>
>>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@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,
>>
>>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel