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

[Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7201 bytes --]



On 28/12/2017 10:03, Thomas Hellstrom wrote:
> Hi, Lukasz!
>
> (Sorry for top-posting).
>
> We have Deepak from our team working on the same subject. I think he's 
> in over the holidays so I'll add him to the CC list.
Great!
>
> Adding damage to the plane state is, IMO the correct way to do it. 
> However, from your proposal it's not clear whether damage is given in 
> the plane-, crtc- or framebuffer coordinates. The last conclusion from 
> our email thread discussion was that they should be given in 
> framebuffer coordinates with helpers to compute plane coordinates or 
> crtc coordinates. The reason being that it's easier for user-space 
> apps to send damage that way, and again, we have the full information 
> that we can clip and scale if necessary. Most drivers probably want 
> the damage in clipped plane- or crtc coordinates. Helpers could for 
> example be in the form of region iterators.
Personally i don't know the difference between plane rects and 
framebuffer rects. I don't know what would be better. I was thinking 
about coordinates of framebuffer that is attached to drm_plane_state.
>
> Full (multi-rect) damage regions are OK with us, although we should 
> keep in mind that we won't be able to compute region unions in the 
> kernel (yet?). Implying: Should we forbid overlapping rects at the 
> interface level or should we just recommend rects not to be 
> overlapping? The former would be pretty hard to enforce efficiently.
I would be for recommendation. We can add some helper functions to 
combine rects and set some limits on number of rects to prevent abuse of 
that interface.
>
> Another thing we should think about is how to use this interface for 
> the legacy "dirtyfb" call. Probably we need to clear the damage 
> property on each state-update, or set a flag that "this is a dirtyfb 
> state update".
>
> IMO we should also have as an end goal of this work to have 
> gnome-shell on drm sending damage regions on page-flip, which means 
> either porting gnome-shell to atomic or set up a new legacy 
> page-flip-with-atomic ioctl.
Can't we reuse dirtyfb ioctl for this purpose? It would be called before 
page_flip ioctl?
>
> /Thomas
>
>
> On 12/21/2017 12:10 PM, 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.
>> +     */
>> +    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;
>>   };
>
>


[-- Attachment #1.2: Type: text/html, Size: 9653 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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