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;
From:   Deepak Singh Rawat <drawat@vmware.com>
To:     Daniel Vetter <daniel@ffwll.ch>
CC:     "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        Thomas Hellstrom <thellstrom@vmware.com>,
        Sinclair Yeh <syeh@vmware.com>,
        linux-graphics-maintainer <linux-graphics-maintainer@vmware.com>,
        "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>,
        "lukasz.spintzyk@displaylink.com" <lukasz.spintzyk@displaylink.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>
Subject: RE: [RFC 3/3] drm: Add helper to validate damage during modeset_check
Thread-Topic: [RFC 3/3] drm: Add helper to validate damage during
 modeset_check
Thread-Index: AQHTzG/R+8rDCCfBbEuJL0AYZYU6cKPxzyyAgAEGd1A=
Date:   Thu, 5 Apr 2018 23:55:29 +0000
Message-ID: <MWHPR05MB3117BE16BBCC51D394185510BABB0@MWHPR05MB3117.namprd05.prod.outlook.com>
References: <1522885748-67122-1-git-send-email-drawat@vmware.com>
 <1522885748-67122-4-git-send-email-drawat@vmware.com>
 <20180405075929.GR3881@phenom.ffwll.local>
In-Reply-To: <20180405075929.GR3881@phenom.ffwll.local>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: a290e4ce-7e03-4cd6-5311-08d59b50b619
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Apr 2018 23:55:29.6754
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR05MB3312
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>=20
> On Wed, Apr 04, 2018 at 04:49:08PM -0700, Deepak Rawat wrote:
> > This patch adds a helper which should be called by driver which enable
> > damage (by calling drm_plane_enable_damage_clips) from atomic_check
> > hook. This helper for now set the damage to NULL for the planes on crtc
> > which need full modeset.
> >
> > The driver also need to check for other crtc properties which can
> > affect damage in atomic_check hook, like gamma, ctm, etc. Plane related
> > properties which affect damage can be handled in damage iterator.
> >
> > Signed-off-by: Deepak Rawat <drawat@vmware.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 47
> +++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic_helper.h     |  2 ++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> > index 355b514..23f44ab 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -3987,3 +3987,50 @@ drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> >  	return true;
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
> > +
> > +/**
> > + * drm_atomic_helper_check_damage - validate state object for damage
> changes
> > + * @dev: DRM device
> > + * @state: the driver state object
> > + *
> > + * Check the state object if damage is required or not. In case damage=
 is
> not
> > + * required e.g. need modeset, the damage blob is set to NULL.
>=20
> Why is that needed?
>=20
> I can imagine that drivers unconditionally upload everything for a
> modeset, and hence don't need special damage tracking. But for that it's
> imo better to have a clear_damage() helper.

Don't we need a generic helper which all drivers can use to see if anything
in drm_atomic_state will result in full update? My intention for calling th=
at
function from atomic_check hook was because state access can
return -EDEADLK.

I think for now having drm_damage_helper_clear_damage helper and=20
calling it from atomic_check seems better option. In future once I have
clear idea, a generic function can be done.


>=20
> But even for modesets (e.g. resolution changes) I'd expect that virtual
> drivers don't want to upload unecessary amounts of data. Manual upload
> hw drivers probably need to upload everything, because the panel will hav=
e
> forgotten all the old data once power cycled.

AFAIK current vmwgfx will do full upload for resolution change.

>=20
> And if you think this is really the right thing, then we need to rename
> the function to tell what it does, e.g.
>=20
> drm_damage_helper_clear_damage_on_modeset()
>=20
> drm_damage_helper because I think we should stuff this all into
> drm_damage_helper.c, see previous patch.
>=20
> But I think a
>=20
> drm_damage_helper_clear_damage(crtc_state)
>=20
> which you can use in your crtc->atomic_check function like
>=20
> crtc_atomic_check(state)
> {
> 	if (drm_atomic_crtc_needs_modeset(state))
> 		drm_damage_helper_clear_damage(state);
> }
>=20
> is more flexible and useful for drivers. There might be other cases where
> clearing damage is the right thing to do. Also, there's the question of
> whether no damage clips =3D=3D full damage or not, so maybe we should cal=
l
> this helper full_damage() instead.

In my opinion if via proper documentation it is conveyed that no damage
means full-update the clear_damage makes sense.

> -Daniel
>=20
> > + *
> > + * NOTE: This helper is not called by core. Those driver which enable
> damage
> > + * using drm_plane_enable_damage_clips should call this from their
> @atomic_check
> > + * hook.
> > + */
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > +				   struct drm_atomic_state *state)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_plane *plane;
> > +	struct drm_crtc_state *crtc_state;
> > +	unsigned plane_mask;
> > +	int i;
> > +
> > +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> > +		if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > +			continue;
> > +
> > +		plane_mask =3D crtc_state->plane_mask;
> > +		plane_mask |=3D crtc->state->plane_mask;
> > +
> > +		drm_for_each_plane_mask(plane, dev, plane_mask) {
> > +			struct drm_plane_state *plane_state =3D
> > +				drm_atomic_get_plane_state(state, plane);
> > +
> > +			if (IS_ERR(plane_state))
> > +				return PTR_ERR(plane_state);
> > +
> > +			if (plane_state->damage_clips) {
> > +				drm_property_blob_put(plane_state-
> >damage_clips);
> > +				plane_state->damage_clips =3D NULL;
> > +				plane_state->num_clips =3D 0;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_atomic_helper_check_damage);
> > diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h
> > index ebd4b66..b12335c 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -224,6 +224,8 @@ drm_atomic_helper_damage_iter_init(struct
> drm_atomic_helper_damage_iter *iter,
> >  bool
> >  drm_atomic_helper_damage_iter_next(struct
> drm_atomic_helper_damage_iter *iter,
> >  				   struct drm_rect *rect);
> > +int drm_atomic_helper_check_damage(struct drm_device *dev,
> > +				   struct drm_atomic_state *state);
> >
> >  /**
> >   * drm_atomic_crtc_for_each_plane - iterate over planes currently
> attached to CRTC
> > --
> > 2.7.4
> >
>=20
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://urldefense.proofpoint.com/v2/url?u=3Dhttp-
> 3A__blog.ffwll.ch&d=3DDwIBAg&c=3DuilaK90D4TOVoH58JNXRgQ&r=3DzOOG28inJK
> 0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw&m=3D70sQYwsOsrWAPt1SdaK8gDC1Fr3KTUpJLw
> 008Coi8rY&s=3DwCKqHwASJhJBVWlirJDaofj0YDju_QHCPE4uZw8W3Mg&e=3D