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

[Thomas Hellstrom <thellstrom@vmware.com>]

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

/Thomas


>> Thanks,
>> Deepak
>>
>>
>>> And when we discussed this iirc we've identified a clear need for at least
>>> some drivers to deal in crtc dirty rectangles. I think the initial core
>>> support should include a helper which takes an atomic update for a given
>>> crtc, and converts all the plane dirty rectangles into crtc rectangles.
>>> Including any full-plane or full-crtc upgrades needed due to e.g.
>>> reposition, changed gamma, changed blendign/zpos or anything else really
>>> that would affect the entire plane respectively crtc. That would also
>>> provide a really good model for what damage actually means.
>>>
>>> Plus ofc we need userspace for this, preferrably as a patch to something
>>> generic like weston or xfree86-video-modesetting. And an example kernel
>>> implementation.
>>>
>>> Cheers, Daniel
>>>
>>>>> +	 */
>>>>> +	struct drm_property *dirty_rects_property;
>>>>>   	/**
>>>>>   	 * @prop_active: Default atomic CRTC property to control the active
>>>>>   	 * state, which is the simplified implementation for DPMS in atomic
>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>> index 8185e3468a23..7d45b164ccce 100644
>>>>> --- a/include/drm/drm_plane.h
>>>>> +++ b/include/drm/drm_plane.h
>>>>> @@ -131,6 +131,9 @@ struct drm_plane_state {
>>>>>   	 */
>>>>>   	struct drm_crtc_commit *commit;
>>>>>
>>>>> +	/* Optional blob property with damaged regions. */
>>>>> +	struct drm_property_blob *dirty_blob;
>>>>> +
>>>>>   	struct drm_atomic_state *state;
>>>>>   };
>>>>>
>>>>> --
>>>>> 2.15.1
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-
>>> 3A__lists.freedesktop.org_mailman_listinfo_dri-
>>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
>>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
>>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
>>>> --
>>>> Ville Syrjälä
>>>> Intel OTC
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://urldefense.proofpoint.com/v2/url?u=https-
>>> 3A__lists.freedesktop.org_mailman_listinfo_dri-
>>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
>>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
>>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> https://urldefense.proofpoint.com/v2/url?u=http-
>>> 3A__blog.ffwll.ch&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK
>>> 0762SxAf-cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=YOIl9T_34ABdrpqBnf2-
>>> IBYMRBEEBmnUTRTo7czA810&e=
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://urldefense.proofpoint.com/v2/url?u=https-
>>> 3A__lists.freedesktop.org_mailman_listinfo_dri-
>>> 2Ddevel&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=zOOG28inJK0762SxAf
>>> -cyZdStnd2NQpRu98lJP2iYGw&m=04r8F-
>>> 0I9tzWCGiUJfjTdf0EjFqdU75F7Ya45iF184Q&s=-
>>> PsVynoB9beTmqss0WTxKi0L3nmOe_lcFKabpb55jaE&e=


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