Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane
To:     Deepak Rawat <drawat@vmware.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "thellstrom@vmware.com" <thellstrom@vmware.com>,
        "syeh@vmware.com" <syeh@vmware.com>
CC:     "linux-graphics-maintainer@vmware.com" 
        <linux-graphics-maintainer@vmware.com>,
        "daniel@ffwll.ch" <daniel@ffwll.ch>,
        "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
        "noralf@tronnes.org" <noralf@tronnes.org>,
        "robdclark@gmail.com" <robdclark@gmail.com>,
        "gustavo@padovan.org" <gustavo@padovan.org>,
        "maarten.lankhorst@linux.intel.com" 
        <maarten.lankhorst@linux.intel.com>,
        "seanpaul@chromium.org" <seanpaul@chromium.org>,
        "airlied@linux.ie" <airlied@linux.ie>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <1522885748-67122-1-git-send-email-drawat@vmware.com>
 <1522885748-67122-2-git-send-email-drawat@vmware.com>
From:   Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
Message-ID: <0e7c70bc-6587-41ea-263a-68018bcb7db2@displaylink.com>
Date:   Tue, 10 Apr 2018 10:10:40 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <1522885748-67122-2-git-send-email-drawat@vmware.com>
Content-Language: en-US
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2018 08:10:45.1802 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: dcdeb224-807f-49a0-7554-08d59eba9037
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: a4bda75a-b444-4312-9c90-44a7c4b2c91a
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0701MB2914
X-MC-Unique: Btv7ezxzN06NdcplPlmPjA-1
Content-Type: text/plain; charset=WINDOWS-1252; format=flowed
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 05/04/2018 01:49, Deepak Rawat wrote:
> From: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
>
> Optional plane property to mark damaged regions on the plane in
> framebuffer coordinates of the framebuffer attached to the plane.
>
> The layout of blob data is simply an array of drm_mode_rect with maximum
> array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike plane src
> coordinates, damage clips are not in 16.16 fixed point.
>
> Damage clips are a hint to kernel as which area of framebuffer has
> changed since last page-flip. This should be helpful for some drivers
> especially for virtual devices where each framebuffer change needs to
> be transmitted over network, usb, etc.
>
> Driver which are interested in enabling DAMAGE_CLIPS property for a
> plane should enable this property using drm_plane_enable_damage_clips.
>
> Signed-off-by: Lukasz Spintzyk <lukasz.spintzyk@displaylink.com>
> Signed-off-by: Deepak Rawat <drawat@vmware.com>
> ---
>   drivers/gpu/drm/drm_atomic.c        | 42 ++++++++++++++++++++++++++++++=
+++++++
>   drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
>   drivers/gpu/drm/drm_mode_config.c   |  5 +++++
>   drivers/gpu/drm/drm_plane.c         | 12 +++++++++++
>   include/drm/drm_mode_config.h       | 15 +++++++++++++
>   include/drm/drm_plane.h             | 16 ++++++++++++++
>   include/uapi/drm/drm_mode.h         | 15 +++++++++++++
>   7 files changed, 109 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d25c42..9226d24 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -669,6 +669,40 @@ static void drm_atomic_crtc_print_state(struct drm_p=
rinter *p,
>   }
>  =20
>   /**
> + * drm_atomic_set_damage_for_plane - sets the damage clips property to p=
lane
> + * @state: plane state
> + * @blob: damage clips in framebuffer coordinates
> + *
> + * Returns:
> + *
> + * Zero on success, error code on failure.
> + */
> +static int drm_atomic_set_damage_for_plane(struct drm_plane_state *state=
,
> +=09=09=09=09=09   struct drm_property_blob *blob)
> +{
> +=09if (blob =3D=3D state->damage_clips)
> +=09=09return 0;
> +
> +=09drm_property_blob_put(state->damage_clips);
> +=09state->damage_clips =3D NULL;
> +
> +=09if (blob) {
> +=09=09uint32_t count =3D blob->length/sizeof(struct drm_rect);
> +
> +=09=09if (count > DRM_MODE_FB_DIRTY_MAX_CLIPS)
> +=09=09=09return -EINVAL;
> +
> +=09=09state->damage_clips =3D drm_property_blob_get(blob);
> +=09=09state->num_clips =3D count;
> +=09} else {
> +=09=09state->damage_clips =3D NULL;
> +=09=09state->num_clips =3D 0;
> +=09}
> +
> +=09return 0;
> +}
> +
> +/**
>    * drm_atomic_get_plane_state - get plane state
>    * @state: global atomic state object
>    * @plane: plane to get state object for
> @@ -793,6 +827,12 @@ static int drm_atomic_plane_set_property(struct drm_=
plane *plane,
>   =09=09state->color_encoding =3D val;
>   =09} else if (property =3D=3D plane->color_range_property) {
>   =09=09state->color_range =3D val;
> +=09} else if (property =3D=3D config->prop_damage_clips) {
> +=09=09struct drm_property_blob *blob =3D
> +=09=09=09drm_property_lookup_blob(dev, val);
> +=09=09int ret =3D drm_atomic_set_damage_for_plane(state, blob);
> +=09=09drm_property_blob_put(blob);
> +=09=09return ret;
>   =09} else if (plane->funcs->atomic_set_property) {
>   =09=09return plane->funcs->atomic_set_property(plane, state,
>   =09=09=09=09property, val);
> @@ -856,6 +896,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane=
,
>   =09=09*val =3D state->color_encoding;
>   =09} else if (property =3D=3D plane->color_range_property) {
>   =09=09*val =3D state->color_range;
> +=09} else if (property =3D=3D config->prop_damage_clips) {
> +=09=09*val =3D (state->damage_clips) ? state->damage_clips->base.id : 0;
>   =09} else if (plane->funcs->atomic_get_property) {
>   =09=09return plane->funcs->atomic_get_property(plane, state, property, =
val);
>   =09} else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_at=
omic_helper.c
> index c356545..55b44e3 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3506,6 +3506,8 @@ void __drm_atomic_helper_plane_duplicate_state(stru=
ct drm_plane *plane,
>  =20
>   =09state->fence =3D NULL;
>   =09state->commit =3D NULL;
> +=09state->damage_clips =3D NULL;
> +=09state->num_clips =3D 0;
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>  =20
> @@ -3550,6 +3552,8 @@ void __drm_atomic_helper_plane_destroy_state(struct=
 drm_plane_state *state)
>  =20
>   =09if (state->commit)
>   =09=09drm_crtc_commit_put(state->commit);
> +
> +=09drm_property_blob_put(state->damage_clips);
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  =20
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode=
_config.c
> index e5c6533..e93b127 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -293,6 +293,11 @@ static int drm_mode_create_standard_properties(struc=
t drm_device *dev)
>   =09=09return -ENOMEM;
>   =09dev->mode_config.prop_crtc_id =3D prop;
>  =20
> +=09prop =3D drm_property_create(dev, DRM_MODE_PROP_BLOB, "DAMAGE_CLIPS",=
 0);
> +=09if (!prop)
> +=09=09return -ENOMEM;
> +=09dev->mode_config.prop_damage_clips =3D prop;
> +
>   =09prop =3D drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>   =09=09=09"ACTIVE");
>   =09if (!prop)
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 6d2a6e4..071221b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1101,3 +1101,15 @@ int drm_mode_page_flip_ioctl(struct drm_device *de=
v,
>  =20
>   =09return ret;
>   }
> +
> +/**
> + * drm_plane_enable_damage_clips - enable damage clips property
> + * @plane: plane on which this property to enable.
> + */
> +void drm_plane_enable_damage_clips(struct drm_plane *plane)
> +{
> +=09struct drm_device *dev =3D plane->dev;
> +=09struct drm_mode_config *config =3D &dev->mode_config;
> +
> +=09drm_object_attach_property(&plane->base, config->prop_damage_clips, 0=
);
> +}
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.=
h
> index 7569f22..d8767da 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -628,6 +628,21 @@ struct drm_mode_config {
>   =09 */
>   =09struct drm_property *prop_crtc_id;
>   =09/**
> +=09 * @prop_damage_clips: Optional plane property to mark damaged region=
s
> +=09 * on the plane in framebuffer coordinates of the framebuffer attache=
d
> +=09 * to the plane.
> +=09 *
> +=09 * The layout of blob data is simply an array of drm_mode_rect with
> +=09 * maximum array size limited by DRM_MODE_FB_DIRTY_MAX_CLIPS. Unlike
> +=09 * plane src coordinates, damage clips are not in 16.16 fixed point.
> +=09 *
> +=09 * Damage clips are a hint to kernel as which area of framebuffer has
> +=09 * changed since last page-flip. This should be helpful
> +=09 * for some drivers especially for virtual devices where each
> +=09 * framebuffer change needs to be transmitted over network, usb, etc.
> +=09 */
> +=09struct drm_property *prop_damage_clips;
> +=09/**
>   =09 * @prop_active: Default atomic CRTC property to control the active
>   =09 * state, which is the simplified implementation for DPMS in atomic
>   =09 * drivers.
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index f7bf4a4..9f24548 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -146,6 +146,21 @@ struct drm_plane_state {
>   =09 */
>   =09struct drm_crtc_commit *commit;
>  =20
> +=09/*
> +=09 * @damage_clips
> +=09 *
> +=09 * blob property with damage as array of drm_rect in framebuffer
> +=09 * coodinates.
> +=09 */
> +=09struct drm_property_blob *damage_clips;
> +
> +=09/*
> +=09 * @num_clips
> +=09 *
> +=09 * Number of drm_rect in @damage_clips.
> +=09 */
> +=09uint32_t num_clips;
> +
>   =09struct drm_atomic_state *state;
>   };
>  =20
> @@ -611,6 +626,7 @@ int drm_plane_init(struct drm_device *dev,
>   =09=09   const uint32_t *formats, unsigned int format_count,
>   =09=09   bool is_primary);
>   void drm_plane_cleanup(struct drm_plane *plane);
> +void drm_plane_enable_damage_clips(struct drm_plane *plane);
>  =20
>   /**
>    * drm_plane_index - find the index of a registered plane
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 50bcf42..0ad0d5b 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -873,6 +873,21 @@ struct drm_mode_revoke_lease {
>   =09__u32 lessee_id;
>   };
>  =20
> +/**
> + * struct drm_mode_rect - two dimensional rectangle drm_rect exported to
> + * user-space.
> + * @x1: horizontal starting coordinate (inclusive)
> + * @y1: vertical starting coordinate (inclusive)
> + * @x2: horizontal ending coordinate (exclusive)
> + * @y2: vertical ending coordinate (exclusive)
> + */
> +struct drm_mode_rect {
> +=09__s32 x1;
> +=09__s32 y1;
> +=09__s32 x2;
> +=09__s32 y2;
> +};
I wonder why we can't use move 'struct drm_rect'=A0 definition from=20
'include/drm/drm_rect.h'
and include 'uapi/drm/drm_mode.h' in private header=20
'include/drm/drm_rect.h'.
Is there any general rule that disallows it?
> +
>   #if defined(__cplusplus)
>   }
>   #endif