Re: [PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane

[Thomas Hellstrom <thellstrom@vmware.com>]

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@lists.freedesktop.org] On Behalf
>>>>> Of Daniel Vetter
>>>>> Sent: Thursday, December 21, 2017 5:11 AM
>>>>> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Cc: airlied@linux.ie; daniel.vetter@intel.com; dri-
>>>>> devel@lists.freedesktop.org; Lukasz Spintzyk
>>>>> <lukasz.spintzyk@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@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
>

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