Re: [PATCH] drm/modes: Support video parameters with only reflect option

[Stephan Gerhold <stephan@gerhold.net>]

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