On 08/09/2016 04:08 PM, Daniel Vetter wrote:
> On Tue, Aug 9, 2016 at 3:59 PM, Thomas Hellstrom <thellstrom@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>>> Date: Thu Dec 3 23:25:47 2009 +0000
>>>
>>> drm: Add dirty ioctl and property
>>>
>>> Cc: Jakob Bornecrantz <jakob@xxxxxxxxxx>
>>> Cc: Dave Airlie <airlied@xxxxxxxxxx>
>>> Cc: Sinclair Yeh <syeh@xxxxxxxxxx>
>>> Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>>> ---
>>> 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,
>>
>>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
