linux-kernel - Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic modesetting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190918201617.5gwzmshoxbcxbmrx@gilmour>
Date:   Wed, 18 Sep 2019 22:16:17 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        Chen-Yu Tsai <wens@...e.org>, dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: sun8i-ui/vi: Fix layer zpos change/atomic
 modesetting

On Wed, Sep 18, 2019 at 05:23:09PM +0200, Ondřej Jirman wrote:
> Hi,
>
> On Wed, Sep 18, 2019 at 04:17:34PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Sun, Sep 15, 2019 at 12:03:37AM +0200, megous@...ous.com wrote:
> > > From: Ondrej Jirman <megous@...ous.com>
> > >
> > > There are various issues that this re-work of sun8i_[uv]i_layer_enable
> > > function fixes:
> > >
> > > - Make sure that we re-initialize zpos on reset
> > > - Minimize register updates by doing them only when state changes
> > > - Fix issue where DE pipe might get disabled even if it is no longer
> > >   used by the layer that's currently calling sun8i_ui_layer_enable
> > > - .atomic_disable callback is not really needed because .atomic_update
> > >   can do the disable too, so drop the duplicate code
> > >
> > > Signed-off-by: Ondrej Jirman <megous@...ous.com>
> >
> > It looks like these fixes should be in separate patches. Is there any
> > reason it's not the case?
>
> Bullet points just describe the resulting effect/benefits of the change to fix
> the pipe control register update issue (see the referenced e-mail).

It's definitely ok to have multiple patches needed to address a single
perceived issue.

A commit is not about what you're fixing but what you're changing. And
the fact that you have tha bullet list in the first place proves that
you have multiple logical changes in your patch.

And even then, your commit log mentions that you're fixing multiple
issues (without explaining them).

> I can maybe split off the first bullet point into a separate patch. But
> I can't guarantee it will not make the original issue worse, because it might
> have been hiding the other issue with register updates.
>
> The rest is just a result of the single logical change. It doesn't work
> individually, it all has the goal of fixing the issue as a whole.
>
> If I were to split it I would have to actually re-implement .atomic_disable
> callback only to remove it in the next patch. I don't see the benefit.

Your commit log says that you remove atomic_disable. Why would you
remove it, to add it back, to remove it again?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)