[PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

[PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

[Peteris Rudzusiks]

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@gmail.com>
---
 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

[Ben Skeggs]

On Sun, 12 May 2019 at 04:23, Peteris Rudzusiks
<peteris.rudzusiks@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@gmail.com>
Oops, nice catch.  Thank you for this, I've merged it in my tree and
will get it upstream ASAP.

Thanks,
Ben.

> ---
>  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

[Peteris Rudzusiks]

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@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@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

[Ilia Mirkin]

On Tue, May 14, 2019 at 3:57 PM Peteris Rudzusiks
<peteris.rudzusiks@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@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/nouveau: fix duplication of nv50_head_atom struct

[Peteris Rudzusiks]

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@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@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@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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau