[PATCH 1/1] drm: Add dirty_rects atomic blob property for drm_plane
Thomas Hellstrom
thellstrom at vmware.com
Mon Mar 5 09:22:09 UTC 2018
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...
/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 at 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 at 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 at 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=
More information about the dri-devel
mailing list