Nouveau - May 2019 - [PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

On Tue, May 14, 2019 at 04:55:05PM +1000, Ben Skeggs
wrote:
> On Sun, 12 May 2019 at 04:23, Peteris Rudzusiks
> <peteris.rudzusiks at gmail.com> wrote:
> >
> > nv50_head_atomic_duplicate_state() makes a copy of nv50_head_atom
> > struct. This patch adds copying of struct member named "or",
which
> > previously was left uninitialized in the duplicated structure.
> >
> > Due to this bug, incorrect nhsync and nvsync values were sometimes
used.
> > In my particular case, that lead to a mismatch between the output
> > resolution of the graphics device (GeForce GT 630 OEM) and the
reported
> > input signal resolution on the display. xrandr reported 1680x1050, but
> > the display reported 1280x1024. As a result of this mismatch, the
output
> > on the display looked like it was cropped (only part of the output was
> > actually visible on the display).
> >
> > git bisect pointed to commit 2ca7fb5c1cc6 ("drm/nouveau/kms/nv50:
handle
> > SetControlOutputResource from head"), which added the member
"or" to
> > nv50_head_atom structure, but forgot to copy it in
> > nv50_head_atomic_duplicate_state().
> >
> > Fixes: 2ca7fb5c1cc6 ("drm/nouveau/kms/nv50: handle
SetControlOutputResource from head")
> > Signed-off-by: Peteris Rudzusiks <peteris.rudzusiks at
gmail.com>
> Oops, nice catch.  Thank you for this, I've merged it in my tree and
> will get it upstream ASAP.
> 
> Thanks,
> Ben.
> 
Hi Ben,

Thank you for taking the time to review and merge this patch.

I'm new to the Linux kernel development process, so I am not sure what
happens next. Does inclusion in your tree imply that this fix will end
up in some (most likely - next) mainline kernel? Will it also be
backported to 4.19 LTS branch?

This bug affects all kernel versions starting from v4.18. Probably not
that many systems though.

Cheers,
Peteris
> > ---
> >  drivers/gpu/drm/nouveau/dispnv50/head.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c
b/drivers/gpu/drm/nouveau/dispnv50/head.c
> > index 2e7a0c347ddb..adce62f4e18f 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/head.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
> > @@ -413,6 +413,7 @@ nv50_head_atomic_duplicate_state(struct drm_crtc
*crtc)
> >         asyh->ovly = armh->ovly;
> >         asyh->dither = armh->dither;
> >         asyh->procamp = armh->procamp;
> > +       asyh->or = armh->or;
> >         asyh->dp = armh->dp;
> >         asyh->clr.mask = 0;
> >         asyh->set.mask = 0;
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, May 14, 2019 at 3:57 PM Peteris Rudzusiks
<peteris.rudzusiks at gmail.com> wrote:
>
> On Tue, May 14, 2019 at 04:55:05PM +1000, Ben Skeggs wrote:
> > On Sun, 12 May 2019 at 04:23, Peteris Rudzusiks
> > <peteris.rudzusiks at gmail.com> wrote:
> > >
> > > nv50_head_atomic_duplicate_state() makes a copy of nv50_head_atom
> > > struct. This patch adds copying of struct member named
"or", which
> > > previously was left uninitialized in the duplicated structure.
> > >
> > > Due to this bug, incorrect nhsync and nvsync values were
sometimes used.
> > > In my particular case, that lead to a mismatch between the output
> > > resolution of the graphics device (GeForce GT 630 OEM) and the
reported
> > > input signal resolution on the display. xrandr reported
1680x1050, but
> > > the display reported 1280x1024. As a result of this mismatch, the
output
> > > on the display looked like it was cropped (only part of the
output was
> > > actually visible on the display).
> > >
> > > git bisect pointed to commit 2ca7fb5c1cc6
("drm/nouveau/kms/nv50: handle
> > > SetControlOutputResource from head"), which added the member
"or" to
> > > nv50_head_atom structure, but forgot to copy it in
> > > nv50_head_atomic_duplicate_state().
> > >
> > > Fixes: 2ca7fb5c1cc6 ("drm/nouveau/kms/nv50: handle
SetControlOutputResource from head")
> > > Signed-off-by: Peteris Rudzusiks <peteris.rudzusiks at
gmail.com>
> > Oops, nice catch.  Thank you for this, I've merged it in my tree
and
> > will get it upstream ASAP.
> >
> > Thanks,
> > Ben.
> >
> Hi Ben,
>
> Thank you for taking the time to review and merge this patch.
>
> I'm new to the Linux kernel development process, so I am not sure what
> happens next. Does inclusion in your tree imply that this fix will end
> up in some (most likely - next) mainline kernel? Will it also be
> backported to 4.19 LTS branch?
>
> This bug affects all kernel versions starting from v4.18. Probably not
> that many systems though.
Ben submits a pull request to Dave Airlie (drm maintainer), and Dave
submits one to Linus for inclusion in the "official" upstream
repository. Dave just sent a pull request to Linus, who usually picks
these up within a few days (exceptions apply).

Once in the mainline tree, the "Fixes" tag is likely to cause it to
get picked up for stable. You can also nominate it for stable kernel
branch inclusion explicitly (there are instructions somewhere, but
basically you send an email to some list saying "please include commit
ABC in kernels XYZ").

What Ubuntu ships is, ultimately, up to Ubuntu. They will, however,
frequently follow the stable kernel branches, and listen to the list
above as well.

Hope this helps,

  -ilia

On Wed, May 15, 2019 at 11:29:40PM -0400, Ilia Mirkin
wrote:
> On Tue, May 14, 2019 at 3:57 PM Peteris Rudzusiks
> <peteris.rudzusiks at gmail.com> wrote:
> >
> > On Tue, May 14, 2019 at 04:55:05PM +1000, Ben Skeggs wrote:
> > > On Sun, 12 May 2019 at 04:23, Peteris Rudzusiks
> > > <peteris.rudzusiks at gmail.com> wrote:
> > > >
> > > > nv50_head_atomic_duplicate_state() makes a copy of
nv50_head_atom
> > > > struct. This patch adds copying of struct member named
"or", which
> > > > previously was left uninitialized in the duplicated
structure.
> > > >
> > > > Due to this bug, incorrect nhsync and nvsync values were
sometimes used.
> > > > In my particular case, that lead to a mismatch between the
output
> > > > resolution of the graphics device (GeForce GT 630 OEM) and
the reported
> > > > input signal resolution on the display. xrandr reported
1680x1050, but
> > > > the display reported 1280x1024. As a result of this
mismatch, the output
> > > > on the display looked like it was cropped (only part of the
output was
> > > > actually visible on the display).
> > > >
> > > > git bisect pointed to commit 2ca7fb5c1cc6
("drm/nouveau/kms/nv50: handle
> > > > SetControlOutputResource from head"), which added the
member "or" to
> > > > nv50_head_atom structure, but forgot to copy it in
> > > > nv50_head_atomic_duplicate_state().
> > > >
> > > > Fixes: 2ca7fb5c1cc6 ("drm/nouveau/kms/nv50: handle
SetControlOutputResource from head")
> > > > Signed-off-by: Peteris Rudzusiks <peteris.rudzusiks at
gmail.com>
> > > Oops, nice catch.  Thank you for this, I've merged it in my
tree and
> > > will get it upstream ASAP.
> > >
> > > Thanks,
> > > Ben.
> > >
> > Hi Ben,
> >
> > Thank you for taking the time to review and merge this patch.
> >
> > I'm new to the Linux kernel development process, so I am not sure
what
> > happens next. Does inclusion in your tree imply that this fix will end
> > up in some (most likely - next) mainline kernel? Will it also be
> > backported to 4.19 LTS branch?
> >
> > This bug affects all kernel versions starting from v4.18. Probably not
> > that many systems though.
> 
> Ben submits a pull request to Dave Airlie (drm maintainer), and Dave
> submits one to Linus for inclusion in the "official" upstream
> repository. Dave just sent a pull request to Linus, who usually picks
> these up within a few days (exceptions apply).
> 
> Once in the mainline tree, the "Fixes" tag is likely to cause it
to
> get picked up for stable. You can also nominate it for stable kernel
> branch inclusion explicitly (there are instructions somewhere, but
> basically you send an email to some list saying "please include commit
> ABC in kernels XYZ").
> 
> What Ubuntu ships is, ultimately, up to Ubuntu. They will, however,
> frequently follow the stable kernel branches, and listen to the list
> above as well.
> 
> Hope this helps,
> 
>   -ilia
Thanks for explaing this. I'll wait and see if this patch gets included
in stable releases without explicitly asking for it.

Regards,
Peteris