From: Daniel Vetter <daniel@ffwll.ch>
To: Deepak Singh Rawat <drawat@vmware.com>
Cc: Daniel Vetter <daniel@ffwll.ch>,
"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
Date: Mon, 9 Apr 2018 10:38:53 +0200 [thread overview]
Message-ID: <20180409083853.GO31310@phenom.ffwll.local> (raw)
In-Reply-To: <MWHPR05MB3117BE16BBCC51D394185510BABB0@MWHPR05MB3117.namprd05.prod.outlook.com>
On Thu, Apr 05, 2018 at 11:55:29PM +0000, Deepak Singh Rawat wrote:
> >
> > 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.
> >
> > Why is that needed?
> >
> > 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 that
> function from atomic_check hook was because state access can
> return -EDEADLK.
>
> I think for now having drm_damage_helper_clear_damage helper and
> calling it from atomic_check seems better option. In future once I have
> clear idea, a generic function can be done.
Yeah, if some of the future helpers need to e.g. allocate memory, then we
need to do any necessary prep steps from ->atomic_check.
But this isn't just prep, it clears stuff, so giving it a name that
indicates better what it does seems like a good idea to me. Just make it
clear that this should be called from ->atomic_check callbacks.
> > 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 have
> > forgotten all the old data once power cycled.
>
> AFAIK current vmwgfx will do full upload for resolution change.
>
> >
> > And if you think this is really the right thing, then we need to rename
> > the function to tell what it does, e.g.
> >
> > drm_damage_helper_clear_damage_on_modeset()
> >
> > drm_damage_helper because I think we should stuff this all into
> > drm_damage_helper.c, see previous patch.
> >
> > But I think a
> >
> > drm_damage_helper_clear_damage(crtc_state)
> >
> > which you can use in your crtc->atomic_check function like
> >
> > crtc_atomic_check(state)
> > {
> > if (drm_atomic_crtc_needs_modeset(state))
> > drm_damage_helper_clear_damage(state);
> > }
> >
> > 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 == full damage or not, so maybe we should call
> > 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.
Documentation is the worst documentation. Function names, or just outright
implemented behaviour is much better (e.g. runtime checks). For full
damage if there's no clip rect I think the iterator should implement that
for us.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
next prev parent reply other threads:[~2018-04-09 8:38 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-04 23:49 [RFC 0/3] drm: page-flip with damage Deepak Rawat
2018-04-04 23:49 ` [RFC 1/3] drm: Add DAMAGE_CLIPS property to plane Deepak Rawat
2018-04-05 7:35 ` Daniel Vetter
2018-04-05 9:00 ` Thomas Hellstrom
2018-04-05 10:03 ` Daniel Vetter
2018-04-05 11:35 ` Thomas Hellstrom
2018-04-05 13:47 ` Daniel Vetter
2018-04-05 13:58 ` Thomas Hellstrom
2018-04-05 11:42 ` Thomas Hellstrom
2018-04-05 13:49 ` Daniel Vetter
2018-04-05 23:07 ` Deepak Singh Rawat
2018-04-09 8:33 ` Daniel Vetter
2018-04-09 16:44 ` Deepak Singh Rawat
2018-04-10 8:10 ` Lukasz Spintzyk
2018-04-04 23:49 ` [RFC 2/3] drm: Add helper iterator functions to iterate over plane damage Deepak Rawat
2018-04-05 7:52 ` Daniel Vetter
2018-04-05 8:49 ` Thomas Hellstrom
2018-04-05 10:10 ` Daniel Vetter
2018-04-05 11:51 ` Thomas Hellstrom
2018-04-05 13:52 ` Daniel Vetter
2018-04-05 8:51 ` Thomas Hellstrom
2018-04-05 13:54 ` Daniel Vetter
2018-04-05 23:59 ` Deepak Singh Rawat
2018-04-09 8:35 ` Daniel Vetter
2018-04-05 23:19 ` Deepak Singh Rawat
2018-04-05 17:55 ` Sinclair Yeh
2018-04-04 23:49 ` [RFC 3/3] drm: Add helper to validate damage during modeset_check Deepak Rawat
2018-04-05 7:59 ` Daniel Vetter
2018-04-05 23:55 ` Deepak Singh Rawat
2018-04-09 8:38 ` Daniel Vetter [this message]
2018-04-05 7:19 ` [RFC 0/3] drm: page-flip with damage Daniel Vetter
2018-04-05 18:43 ` Deepak Singh Rawat
[not found] ` <5f3e1c8a-d2b9-41f9-46f6-2b7f8c736de8@displaylink.com>
2018-04-10 18:56 ` Deepak Singh Rawat
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180409083853.GO31310@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=airlied@linux.ie \
--cc=drawat@vmware.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gustavo@padovan.org \
--cc=linux-graphics-maintainer@vmware.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lukasz.spintzyk@displaylink.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=noralf@tronnes.org \
--cc=robdclark@gmail.com \
--cc=seanpaul@chromium.org \
--cc=syeh@vmware.com \
--cc=thellstrom@vmware.com \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).