* [PATCH] drm/modes: Support video parameters with only reflect option
@ 2019-12-09 18:32 Stephan Gerhold
2019-12-10 10:20 ` Maxime Ripard
0 siblings, 1 reply; 8+ messages in thread
From: Stephan Gerhold @ 2019-12-09 18:32 UTC (permalink / raw)
To: Maxime Ripard, Maarten Lankhorst; +Cc: David Airlie, Stephan Gerhold, dri-devel
At the moment, video mode parameters like video=540x960,reflect_x,
(without rotation set) are silently ignored.
One of the reasons for this is that the calculation that
combines the panel_orientation with cmdline->rotation_reflection
does not handle the case when cmdline->rotation_reflection does
not have any rotation set.
(i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
Example:
*rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
The current code does:
panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
sum_rot = (panel_rot + cmdline_rot) % 4;
and therefore:
panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
cmdline_rot = ilog2(0) = -1
sum_rot = (0 + -1) % 4 = -1 % 4 = 3
...
*rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
To prevent cmdline_rot from becoming -1, we need to treat
the rotation as DRM_MODE_ROTATE_0.
On the other hand, there is no need to go through that calculation
at all if no rotation is set in rotation_reflection.
A simple XOR is enough to combine the reflections.
Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
require any special handling (e.g. specific tiling format).
This makes video parameters with only reflect option work correctly.
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
drivers/gpu/drm/drm_client_modeset.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 895b73f23079..cfebce4f19a5 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -859,19 +859,23 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
*/
cmdline = &connector->cmdline_mode;
if (cmdline->specified && cmdline->rotation_reflection) {
- unsigned int cmdline_rest, panel_rest;
- unsigned int cmdline_rot, panel_rot;
- unsigned int sum_rot, sum_rest;
+ if (cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK) {
+ unsigned int cmdline_rest, panel_rest;
+ unsigned int cmdline_rot, panel_rot;
+ unsigned int sum_rot, sum_rest;
- panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
- cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
- sum_rot = (panel_rot + cmdline_rot) % 4;
+ panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
+ cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
+ sum_rot = (panel_rot + cmdline_rot) % 4;
- panel_rest = *rotation & ~DRM_MODE_ROTATE_MASK;
- cmdline_rest = cmdline->rotation_reflection & ~DRM_MODE_ROTATE_MASK;
- sum_rest = panel_rest ^ cmdline_rest;
+ panel_rest = *rotation & ~DRM_MODE_ROTATE_MASK;
+ cmdline_rest = cmdline->rotation_reflection & ~DRM_MODE_ROTATE_MASK;
+ sum_rest = panel_rest ^ cmdline_rest;
- *rotation = (1 << sum_rot) | sum_rest;
+ *rotation = (1 << sum_rot) | sum_rest;
+ } else {
+ *rotation ^= cmdline->rotation_reflection;
+ }
}
/*
@@ -879,7 +883,8 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
* depending on the hardware this may require the framebuffer
* to be in a specific tiling format.
*/
- if ((*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_180 ||
+ if (((*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_0 &&
+ (*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_180) ||
!plane->rotation_property)
return false;
--
2.24.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-09 18:32 [PATCH] drm/modes: Support video parameters with only reflect option Stephan Gerhold
@ 2019-12-10 10:20 ` Maxime Ripard
2019-12-10 10:42 ` Stephan Gerhold
0 siblings, 1 reply; 8+ messages in thread
From: Maxime Ripard @ 2019-12-10 10:20 UTC (permalink / raw)
To: Stephan Gerhold; +Cc: David Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1841 bytes --]
Hi,
On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> At the moment, video mode parameters like video=540x960,reflect_x,
> (without rotation set) are silently ignored.
>
> One of the reasons for this is that the calculation that
> combines the panel_orientation with cmdline->rotation_reflection
> does not handle the case when cmdline->rotation_reflection does
> not have any rotation set.
> (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
>
> Example:
> *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
>
> The current code does:
> panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> sum_rot = (panel_rot + cmdline_rot) % 4;
>
> and therefore:
> panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> cmdline_rot = ilog2(0) = -1
> sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> ...
> *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
>
> So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> To prevent cmdline_rot from becoming -1, we need to treat
> the rotation as DRM_MODE_ROTATE_0.
>
> On the other hand, there is no need to go through that calculation
> at all if no rotation is set in rotation_reflection.
> A simple XOR is enough to combine the reflections.
>
> Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> require any special handling (e.g. specific tiling format).
>
> This makes video parameters with only reflect option work correctly.
>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Thanks for that commit message.
Can you also add a selftest to make sure we don't get a regression in
the future?
Thanks!
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-10 10:20 ` Maxime Ripard
@ 2019-12-10 10:42 ` Stephan Gerhold
2019-12-11 18:10 ` Maxime Ripard
0 siblings, 1 reply; 8+ messages in thread
From: Stephan Gerhold @ 2019-12-10 10:42 UTC (permalink / raw)
To: Maxime Ripard; +Cc: David Airlie, dri-devel
On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote:
> Hi,
>
> On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> > At the moment, video mode parameters like video=540x960,reflect_x,
> > (without rotation set) are silently ignored.
> >
> > One of the reasons for this is that the calculation that
> > combines the panel_orientation with cmdline->rotation_reflection
> > does not handle the case when cmdline->rotation_reflection does
> > not have any rotation set.
> > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> >
> > Example:
> > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> >
> > The current code does:
> > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> > sum_rot = (panel_rot + cmdline_rot) % 4;
> >
> > and therefore:
> > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> > cmdline_rot = ilog2(0) = -1
> > sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> > ...
> > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> >
> > So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> > To prevent cmdline_rot from becoming -1, we need to treat
> > the rotation as DRM_MODE_ROTATE_0.
> >
> > On the other hand, there is no need to go through that calculation
> > at all if no rotation is set in rotation_reflection.
> > A simple XOR is enough to combine the reflections.
> >
> > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> > require any special handling (e.g. specific tiling format).
> >
> > This makes video parameters with only reflect option work correctly.
> >
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>
> Thanks for that commit message.
>
> Can you also add a selftest to make sure we don't get a regression in
> the future?
Can you explain how/where I would add a test for drm_client_rotation()
in drm_client_modeset.c? I'm not familiar with selftests to be honest.
I found test-drm_cmdline_parser.c but that seems to cover only the
cmdline parsing (which is working correctly already).
Thanks,
Stephan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-10 10:42 ` Stephan Gerhold
@ 2019-12-11 18:10 ` Maxime Ripard
2019-12-11 19:08 ` Stephan Gerhold
0 siblings, 1 reply; 8+ messages in thread
From: Maxime Ripard @ 2019-12-11 18:10 UTC (permalink / raw)
To: Stephan Gerhold; +Cc: David Airlie, dri-devel
Hi Stephan,
On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote:
> On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> > > At the moment, video mode parameters like video=540x960,reflect_x,
> > > (without rotation set) are silently ignored.
> > >
> > > One of the reasons for this is that the calculation that
> > > combines the panel_orientation with cmdline->rotation_reflection
> > > does not handle the case when cmdline->rotation_reflection does
> > > not have any rotation set.
> > > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> > >
> > > Example:
> > > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> > > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> > >
> > > The current code does:
> > > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> > > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> > > sum_rot = (panel_rot + cmdline_rot) % 4;
> > >
> > > and therefore:
> > > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> > > cmdline_rot = ilog2(0) = -1
> > > sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> > > ...
> > > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> > >
> > > So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> > > To prevent cmdline_rot from becoming -1, we need to treat
> > > the rotation as DRM_MODE_ROTATE_0.
> > >
> > > On the other hand, there is no need to go through that calculation
> > > at all if no rotation is set in rotation_reflection.
> > > A simple XOR is enough to combine the reflections.
> > >
> > > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> > > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> > > require any special handling (e.g. specific tiling format).
> > >
> > > This makes video parameters with only reflect option work correctly.
> > >
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> >
> > Thanks for that commit message.
> >
> > Can you also add a selftest to make sure we don't get a regression in
> > the future?
>
> Can you explain how/where I would add a test for drm_client_rotation()
> in drm_client_modeset.c? I'm not familiar with selftests to be honest.
>
> I found test-drm_cmdline_parser.c but that seems to cover only the
> cmdline parsing (which is working correctly already).
The cmdline here is the kernel command line. You were mentionning in
your commit log that video=540x960,reflect_x was broken?
But yeah, I meant this one.
Maxime
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-11 18:10 ` Maxime Ripard
@ 2019-12-11 19:08 ` Stephan Gerhold
2019-12-13 9:39 ` Maxime Ripard
0 siblings, 1 reply; 8+ messages in thread
From: Stephan Gerhold @ 2019-12-11 19:08 UTC (permalink / raw)
To: Maxime Ripard; +Cc: David Airlie, dri-devel
On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote:
> Hi Stephan,
>
> On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote:
> > On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote:
> > > Hi,
> > >
> > > On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> > > > At the moment, video mode parameters like video=540x960,reflect_x,
> > > > (without rotation set) are silently ignored.
> > > >
> > > > One of the reasons for this is that the calculation that
> > > > combines the panel_orientation with cmdline->rotation_reflection
> > > > does not handle the case when cmdline->rotation_reflection does
> > > > not have any rotation set.
> > > > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> > > >
> > > > Example:
> > > > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> > > > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> > > >
> > > > The current code does:
> > > > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> > > > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> > > > sum_rot = (panel_rot + cmdline_rot) % 4;
> > > >
> > > > and therefore:
> > > > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> > > > cmdline_rot = ilog2(0) = -1
> > > > sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> > > > ...
> > > > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> > > >
> > > > So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> > > > To prevent cmdline_rot from becoming -1, we need to treat
> > > > the rotation as DRM_MODE_ROTATE_0.
> > > >
> > > > On the other hand, there is no need to go through that calculation
> > > > at all if no rotation is set in rotation_reflection.
> > > > A simple XOR is enough to combine the reflections.
> > > >
> > > > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> > > > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> > > > require any special handling (e.g. specific tiling format).
> > > >
> > > > This makes video parameters with only reflect option work correctly.
> > > >
> > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > >
> > > Thanks for that commit message.
> > >
> > > Can you also add a selftest to make sure we don't get a regression in
> > > the future?
> >
> > Can you explain how/where I would add a test for drm_client_rotation()
> > in drm_client_modeset.c? I'm not familiar with selftests to be honest.
> >
> > I found test-drm_cmdline_parser.c but that seems to cover only the
> > cmdline parsing (which is working correctly already).
>
> The cmdline here is the kernel command line. You were mentionning in
> your commit log that video=540x960,reflect_x was broken?
>
The parameter is parsed correctly and placed into connector->cmdline_mode.
Therefore, not the *parsing* is broken, only the way we try to apply
and merge them with the panel orientation in drm_client_modeset.c.
There are existing test cases for the parsing of parameters similar to
video=540x960,reflect_x, see drm_cmdline_test_hmirror()
in the aforementioned test file.
Maybe my commit message was not as clear as I hoped :)
Thanks,
Stephan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-11 19:08 ` Stephan Gerhold
@ 2019-12-13 9:39 ` Maxime Ripard
2019-12-13 12:16 ` Stephan Gerhold
0 siblings, 1 reply; 8+ messages in thread
From: Maxime Ripard @ 2019-12-13 9:39 UTC (permalink / raw)
To: Stephan Gerhold; +Cc: David Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 3544 bytes --]
Hi Stephan,
On Wed, Dec 11, 2019 at 08:08:39PM +0100, Stephan Gerhold wrote:
> On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote:
> > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote:
> > > On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> > > > > At the moment, video mode parameters like video=540x960,reflect_x,
> > > > > (without rotation set) are silently ignored.
> > > > >
> > > > > One of the reasons for this is that the calculation that
> > > > > combines the panel_orientation with cmdline->rotation_reflection
> > > > > does not handle the case when cmdline->rotation_reflection does
> > > > > not have any rotation set.
> > > > > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> > > > >
> > > > > Example:
> > > > > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> > > > > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> > > > >
> > > > > The current code does:
> > > > > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> > > > > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> > > > > sum_rot = (panel_rot + cmdline_rot) % 4;
> > > > >
> > > > > and therefore:
> > > > > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> > > > > cmdline_rot = ilog2(0) = -1
> > > > > sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> > > > > ...
> > > > > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> > > > >
> > > > > So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> > > > > To prevent cmdline_rot from becoming -1, we need to treat
> > > > > the rotation as DRM_MODE_ROTATE_0.
> > > > >
> > > > > On the other hand, there is no need to go through that calculation
> > > > > at all if no rotation is set in rotation_reflection.
> > > > > A simple XOR is enough to combine the reflections.
> > > > >
> > > > > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> > > > > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> > > > > require any special handling (e.g. specific tiling format).
> > > > >
> > > > > This makes video parameters with only reflect option work correctly.
> > > > >
> > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > >
> > > > Thanks for that commit message.
> > > >
> > > > Can you also add a selftest to make sure we don't get a regression in
> > > > the future?
> > >
> > > Can you explain how/where I would add a test for drm_client_rotation()
> > > in drm_client_modeset.c? I'm not familiar with selftests to be honest.
> > >
> > > I found test-drm_cmdline_parser.c but that seems to cover only the
> > > cmdline parsing (which is working correctly already).
> >
> > The cmdline here is the kernel command line. You were mentionning in
> > your commit log that video=540x960,reflect_x was broken?
> >
>
> The parameter is parsed correctly and placed into connector->cmdline_mode.
> Therefore, not the *parsing* is broken, only the way we try to apply
> and merge them with the panel orientation in drm_client_modeset.c.
>
> There are existing test cases for the parsing of parameters similar to
> video=540x960,reflect_x, see drm_cmdline_test_hmirror()
> in the aforementioned test file.
>
> Maybe my commit message was not as clear as I hoped :)
Ah, I see what you meant by "silently ignored" now :)
Yeah, maybe we can change that by "not taken into account when
applying the mode" or something like that?
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-13 9:39 ` Maxime Ripard
@ 2019-12-13 12:16 ` Stephan Gerhold
2019-12-16 16:26 ` Maxime Ripard
0 siblings, 1 reply; 8+ messages in thread
From: Stephan Gerhold @ 2019-12-13 12:16 UTC (permalink / raw)
To: Maxime Ripard; +Cc: David Airlie, dri-devel
On Fri, Dec 13, 2019 at 10:39:50AM +0100, Maxime Ripard wrote:
> Hi Stephan,
>
> On Wed, Dec 11, 2019 at 08:08:39PM +0100, Stephan Gerhold wrote:
> > On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote:
> > > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote:
> > > > On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> > > > > > At the moment, video mode parameters like video=540x960,reflect_x,
> > > > > > (without rotation set) are silently ignored.
> > > > > >
> > > > > > One of the reasons for this is that the calculation that
> > > > > > combines the panel_orientation with cmdline->rotation_reflection
> > > > > > does not handle the case when cmdline->rotation_reflection does
> > > > > > not have any rotation set.
> > > > > > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> > > > > >
> > > > > > Example:
> > > > > > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> > > > > > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> > > > > >
> > > > > > The current code does:
> > > > > > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> > > > > > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> > > > > > sum_rot = (panel_rot + cmdline_rot) % 4;
> > > > > >
> > > > > > and therefore:
> > > > > > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> > > > > > cmdline_rot = ilog2(0) = -1
> > > > > > sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> > > > > > ...
> > > > > > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> > > > > >
> > > > > > So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> > > > > > To prevent cmdline_rot from becoming -1, we need to treat
> > > > > > the rotation as DRM_MODE_ROTATE_0.
> > > > > >
> > > > > > On the other hand, there is no need to go through that calculation
> > > > > > at all if no rotation is set in rotation_reflection.
> > > > > > A simple XOR is enough to combine the reflections.
> > > > > >
> > > > > > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> > > > > > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> > > > > > require any special handling (e.g. specific tiling format).
> > > > > >
> > > > > > This makes video parameters with only reflect option work correctly.
> > > > > >
> > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > >
> > > > > Thanks for that commit message.
> > > > >
> > > > > Can you also add a selftest to make sure we don't get a regression in
> > > > > the future?
> > > >
> > > > Can you explain how/where I would add a test for drm_client_rotation()
> > > > in drm_client_modeset.c? I'm not familiar with selftests to be honest.
> > > >
> > > > I found test-drm_cmdline_parser.c but that seems to cover only the
> > > > cmdline parsing (which is working correctly already).
> > >
> > > The cmdline here is the kernel command line. You were mentionning in
> > > your commit log that video=540x960,reflect_x was broken?
> > >
> >
> > The parameter is parsed correctly and placed into connector->cmdline_mode.
> > Therefore, not the *parsing* is broken, only the way we try to apply
> > and merge them with the panel orientation in drm_client_modeset.c.
> >
> > There are existing test cases for the parsing of parameters similar to
> > video=540x960,reflect_x, see drm_cmdline_test_hmirror()
> > in the aforementioned test file.
> >
> > Maybe my commit message was not as clear as I hoped :)
>
> Ah, I see what you meant by "silently ignored" now :)
>
> Yeah, maybe we can change that by "not taken into account when
> applying the mode" or something like that?
Sure, I will try to clarify it for v2.
Anything else I should change?
Thanks,
Stephan
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/modes: Support video parameters with only reflect option
2019-12-13 12:16 ` Stephan Gerhold
@ 2019-12-16 16:26 ` Maxime Ripard
0 siblings, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2019-12-16 16:26 UTC (permalink / raw)
To: Stephan Gerhold; +Cc: David Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 4101 bytes --]
On Fri, Dec 13, 2019 at 01:16:19PM +0100, Stephan Gerhold wrote:
> On Fri, Dec 13, 2019 at 10:39:50AM +0100, Maxime Ripard wrote:
> > Hi Stephan,
> >
> > On Wed, Dec 11, 2019 at 08:08:39PM +0100, Stephan Gerhold wrote:
> > > On Wed, Dec 11, 2019 at 07:10:46PM +0100, Maxime Ripard wrote:
> > > > On Tue, Dec 10, 2019 at 11:42:37AM +0100, Stephan Gerhold wrote:
> > > > > On Tue, Dec 10, 2019 at 11:20:46AM +0100, Maxime Ripard wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Dec 09, 2019 at 07:32:54PM +0100, Stephan Gerhold wrote:
> > > > > > > At the moment, video mode parameters like video=540x960,reflect_x,
> > > > > > > (without rotation set) are silently ignored.
> > > > > > >
> > > > > > > One of the reasons for this is that the calculation that
> > > > > > > combines the panel_orientation with cmdline->rotation_reflection
> > > > > > > does not handle the case when cmdline->rotation_reflection does
> > > > > > > not have any rotation set.
> > > > > > > (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> > > > > > >
> > > > > > > Example:
> > > > > > > *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
> > > > > > > cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> > > > > > >
> > > > > > > The current code does:
> > > > > > > panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> > > > > > > cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> > > > > > > sum_rot = (panel_rot + cmdline_rot) % 4;
> > > > > > >
> > > > > > > and therefore:
> > > > > > > panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
> > > > > > > cmdline_rot = ilog2(0) = -1
> > > > > > > sum_rot = (0 + -1) % 4 = -1 % 4 = 3
> > > > > > > ...
> > > > > > > *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> > > > > > >
> > > > > > > So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> > > > > > > To prevent cmdline_rot from becoming -1, we need to treat
> > > > > > > the rotation as DRM_MODE_ROTATE_0.
> > > > > > >
> > > > > > > On the other hand, there is no need to go through that calculation
> > > > > > > at all if no rotation is set in rotation_reflection.
> > > > > > > A simple XOR is enough to combine the reflections.
> > > > > > >
> > > > > > > Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> > > > > > > DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> > > > > > > require any special handling (e.g. specific tiling format).
> > > > > > >
> > > > > > > This makes video parameters with only reflect option work correctly.
> > > > > > >
> > > > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > > >
> > > > > > Thanks for that commit message.
> > > > > >
> > > > > > Can you also add a selftest to make sure we don't get a regression in
> > > > > > the future?
> > > > >
> > > > > Can you explain how/where I would add a test for drm_client_rotation()
> > > > > in drm_client_modeset.c? I'm not familiar with selftests to be honest.
> > > > >
> > > > > I found test-drm_cmdline_parser.c but that seems to cover only the
> > > > > cmdline parsing (which is working correctly already).
> > > >
> > > > The cmdline here is the kernel command line. You were mentionning in
> > > > your commit log that video=540x960,reflect_x was broken?
> > > >
> > >
> > > The parameter is parsed correctly and placed into connector->cmdline_mode.
> > > Therefore, not the *parsing* is broken, only the way we try to apply
> > > and merge them with the panel orientation in drm_client_modeset.c.
> > >
> > > There are existing test cases for the parsing of parameters similar to
> > > video=540x960,reflect_x, see drm_cmdline_test_hmirror()
> > > in the aforementioned test file.
> > >
> > > Maybe my commit message was not as clear as I hoped :)
> >
> > Ah, I see what you meant by "silently ignored" now :)
> >
> > Yeah, maybe we can change that by "not taken into account when
> > applying the mode" or something like that?
>
> Sure, I will try to clarify it for v2.
>
> Anything else I should change?
The rest looks fine to me.
Thanks!
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-12-16 16:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09 18:32 [PATCH] drm/modes: Support video parameters with only reflect option Stephan Gerhold
2019-12-10 10:20 ` Maxime Ripard
2019-12-10 10:42 ` Stephan Gerhold
2019-12-11 18:10 ` Maxime Ripard
2019-12-11 19:08 ` Stephan Gerhold
2019-12-13 9:39 ` Maxime Ripard
2019-12-13 12:16 ` Stephan Gerhold
2019-12-16 16:26 ` Maxime Ripard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.