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

Thomas Hellstrom thellstrom at vmware.com
Tue Mar 6 07:25:09 UTC 2018


On 03/06/2018 08:08 AM, Daniel Vetter wrote:
> On Mon, Mar 05, 2018 at 11:01:42AM +0100, Thomas Hellstrom wrote:
>> 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.
> Oh, I'd have expected that you folks want fb-coordinate cliprects, since
> you upload the buffers. Otoh the difference is fairly irrelevant if you
> only have one framebuffer that has to be full-screen and unscaled, so not
> exactly sure why you want to transform to crtc coordinates (since for you
> they're the same I'd assume).

> Anyway, if you see I real use for the iterator style I'll of course not
> block it :-) I simply didn't see the use-case, which is why I ignored it.
> And once another driver that wants a single crtc bounding box implements
> support, that team can type the helper infrastructure (that's not on you
> for sure).
>
> I guess we've reached the point where talking in the abstract only leads
> to misunderstanding, and a bit of (driver) code will be the next step.

Agreed!

/Thomas

> -Daniel
>> 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