[PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
Thomas Hellstrom
thellstrom at vmware.com
Mon Mar 5 10:01:42 UTC 2018
On 03/05/2018 10:39 AM, Daniel Vetter wrote:
> On Mon, Mar 05, 2018 at 10:22:09AM +0100, Thomas Hellstrom wrote:
>> On 03/05/2018 09:37 AM, Daniel Vetter wrote:
>>> On Wed, Feb 28, 2018 at 09:10:46PM +0000, Deepak Singh Rawat wrote:
>>>>> -----Original Message-----
>>>>> From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf
>>>>> Of Daniel Vetter
>>>>> Sent: Thursday, December 21, 2017 5:11 AM
>>>>> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> Cc: airlied at linux.ie; daniel.vetter at intel.com; dri-
>>>>> devel at lists.freedesktop.org; Lukasz Spintzyk
>>>>> <lukasz.spintzyk at displaylink.com>
>>>>> Subject: Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for
>>>>> drm_plane
>>>>>
>>>>> On Thu, Dec 21, 2017 at 02:46:55PM +0200, Ville Syrjälä wrote:
>>>>>> On Thu, Dec 21, 2017 at 12:10:08PM +0100, Lukasz Spintzyk wrote:
>>>>>>> Change-Id: I63dce004f8d3c5dc6a7c71070f1fab0707286ea5
>>>>>>> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk at displaylink.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/drm_atomic.c | 10 ++++++++++
>>>>>>> drivers/gpu/drm/drm_mode_config.c | 6 ++++++
>>>>>>> drivers/gpu/drm/drm_plane.c | 1 +
>>>>>>> include/drm/drm_mode_config.h | 5 +++++
>>>>>>> include/drm/drm_plane.h | 3 +++
>>>>>>> 5 files changed, 25 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
>>>>> b/drivers/gpu/drm/drm_atomic.c
>>>>>>> index b76d49218cf1..cd3b4ed7b04c 100644
>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>> @@ -759,6 +759,14 @@ static int drm_atomic_plane_set_property(struct
>>>>> drm_plane *plane,
>>>>>>> state->rotation = val;
>>>>>>> } else if (property == plane->zpos_property) {
>>>>>>> state->zpos = val;
>>>>>>> + } else if (property == config->dirty_rects_property) {
>>>>>>> + bool replaced = false;
>>>>>>> + int ret = drm_atomic_replace_property_blob_from_id(dev,
>>>>>>> + &state->dirty_blob,
>>>>>>> + val,
>>>>>>> + -1,
>>>>>>> + &replaced);
>>>>>>> + return ret;
>>>>>>> } else if (plane->funcs->atomic_set_property) {
>>>>>>> return plane->funcs->atomic_set_property(plane, state,
>>>>>>> property, val);
>>>>>>> @@ -818,6 +826,8 @@ drm_atomic_plane_get_property(struct
>>>>> drm_plane *plane,
>>>>>>> *val = state->rotation;
>>>>>>> } else if (property == plane->zpos_property) {
>>>>>>> *val = state->zpos;
>>>>>>> + } else if (property == config->dirty_rects_property) {
>>>>>>> + *val = (state->dirty_blob) ? state->dirty_blob->base.id : 0;
>>>>>>> } else if (plane->funcs->atomic_get_property) {
>>>>>>> return plane->funcs->atomic_get_property(plane, state,
>>>>> property, val);
>>>>>>> } else {
>>>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>>>> index bc5c46306b3d..d5f1021c6ece 100644
>>>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>>>> @@ -293,6 +293,12 @@ static int
>>>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>>>>> return -ENOMEM;
>>>>>>> dev->mode_config.prop_crtc_id = prop;
>>>>>>>
>>>>>>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>>>>>>> + "DIRTY_RECTS", 0);
>>>>>>> + if (!prop)
>>>>>>> + return -ENOMEM;
>>>>>>> + dev->mode_config.dirty_rects_property = prop;
>>>>>>> +
>>>>>>> prop = drm_property_create_bool(dev,
>>>>> DRM_MODE_PROP_ATOMIC,
>>>>>>> "ACTIVE");
>>>>>>> if (!prop)
>>>>>>> diff --git a/drivers/gpu/drm/drm_plane.c
>>>>> b/drivers/gpu/drm/drm_plane.c
>>>>>>> index 37a93cdffb4a..add110f025e5 100644
>>>>>>> --- a/drivers/gpu/drm/drm_plane.c
>>>>>>> +++ b/drivers/gpu/drm/drm_plane.c
>>>>>>> @@ -258,6 +258,7 @@ int drm_universal_plane_init(struct drm_device
>>>>> *dev, struct drm_plane *plane,
>>>>>>> drm_object_attach_property(&plane->base, config-
>>>>>> prop_src_y, 0);
>>>>>>> drm_object_attach_property(&plane->base, config-
>>>>>> prop_src_w, 0);
>>>>>>> drm_object_attach_property(&plane->base, config-
>>>>>> prop_src_h, 0);
>>>>>>> + drm_object_attach_property(&plane->base, config-
>>>>>> dirty_rects_property, 0);
>>>>>>> }
>>>>>>>
>>>>>>> if (config->allow_fb_modifiers)
>>>>>>> diff --git a/include/drm/drm_mode_config.h
>>>>> b/include/drm/drm_mode_config.h
>>>>>>> index e5f3b43014e1..65f64eb04c0c 100644
>>>>>>> --- a/include/drm/drm_mode_config.h
>>>>>>> +++ b/include/drm/drm_mode_config.h
>>>>>>> @@ -599,6 +599,11 @@ struct drm_mode_config {
>>>>>>> * &drm_crtc.
>>>>>>> */
>>>>>>> struct drm_property *prop_crtc_id;
>>>>>>> + /**
>>>>>>> + * @dirty_rects_property: Optional plane property to mark damaged
>>>>>>> + * regions on the plane framebuffer.
>>>>>> What exactly would the blob contain?
>>>>>>
>>>>>> The comment seems to be implying these are in fb coordiantes as
>>>>>> opposed to plane crtc coordinates? Not sure which would be more
>>>>>> convenient. At least if they're fb coordinates then you probably
>>>>>> want some helpers to translate/rotate/scale those rects to the
>>>>>> crtc coordinates. Actual use depends on the driver/hw I suppose.
>>>>> Yeah I think we also should add a decoded state to the drm_plane_state,
>>>>> which has the full structure and all the details.
>>>> Hi Daniel,
>>>>
>>>> I am working on this functionality to implement the helper functions to
>>>> convert dirty clips from framebuffer coordinates to planes coordinates
>>>> and finally to crtc coordinates.
>>>>
>>>> Is there a reason we should have decoded dirty clips in plane_state ?
>>>> I was thinking to have the clips remain in blob property and decode
>>>> them when needed with the helper function which accept plane state and
>>>> similarly another helper for crtc coordinates. So driver call these helper
>>>> only if there is a need for dirty clips and otherwise can ignore.
>>> Immediately decoding the state blobs is simply best practices, to avoid
>>> duplicated code and subtle differences in driver behaviour. If there's a
>>> good reason for completely different behaviour (like crtc vs plane
>>> coordinate based damage tracking), then decoding using a helper,
>>> on-demand, seems sensible.
>>>
>>> I'd still think we should store the resulting derived state in
>>> drm_crtc/plane_state, like we do for the plane clipping helpers (at least
>>> after Ville's latest patch series has landed).
>>> -Daniel
>> The idea here would be to use a helper iterator to construct the new
>> coordinates needed; the iterator being initialized with the blob.
>>
>> The reason not to store the decoded state is unnecessary duplication. While
>> I guess most drivers would use crtc damage tracking, potentially there might
>> be three coordinate systems used by drivers: Plane fb coordinates, Plane
>> crtc coordinates and crtc coordinates. And typically they'd be used only
>> once...
> Well I was thinking of a helper to compute a single, overall clip rect for
> the entire crtc. Which would then also need to take into account various
> plane state changes, and stuff like background color. For a simple
> coordinate system transform iterators sound like a good idea, but honestly
> I don't expect that to be a common use-case in drivers.
So far, the drivers that have shown interest in this (VMware and
DisplayLink) appear to be mostly interested in getting their hands on
a set of cliprects (rather than a bounding box) in crtc coordinates.
While we recognize the user-space apps will want to hand over the set of
cliprects in fb coordinates. We're also working on a solution where we,
as generically as possible want to be able to attach a remoting server
(like a VNC server) to KMS and that solution would probably want the
same thing. For these things I think an iterator would be a good solution.
However that doesn't really stop us from, based on other driver's use
cases, add additional plane / crtc state and use the iterators to
populate that. Given that, do you think that iterators computing
transformed coordinates would be a reasonable first step? That would
help us enable this path in vmwgfx and start flushing and test page-flip
with damage on real applications and also to figure out a reasonable
atomic counterpart to the dirty ioctl.
Thanks,
Thomas
> Either it's about
> uploading the right dirty data in each plane, or about uploading the right
> final/blended pixels to the screen (in which case all other state changes
> that can affect colors must be taken into account too).
> -Daniel
>
More information about the dri-devel
mailing list