| Message ID | 20191216171017.173326-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/modes: Apply video parameters with only reflect option | expand |
On Mon, Dec 16, 2019 at 06:10:17PM +0100, Stephan Gerhold wrote: > At the moment, video mode parameters like video=540x960,reflect_x, > (without rotation set) are not taken into account when applying the > mode in drm_client_modeset.c. A rotation value without exactly one rotation angle is illegal. IMO the code should not generate a value like that in the first place. > > 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> > --- > v1: https://lists.freedesktop.org/archives/dri-devel/2019-December/248145.html > > Changes in v2: > - Clarified commit message - parameters are parsed correctly, > but not taken into account when applying the mode. > --- > 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.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Dec 16, 2019 at 07:27:25PM +0200, Ville Syrjälä wrote: > On Mon, Dec 16, 2019 at 06:10:17PM +0100, Stephan Gerhold wrote: > > At the moment, video mode parameters like video=540x960,reflect_x, > > (without rotation set) are not taken into account when applying the > > mode in drm_client_modeset.c. > > A rotation value without exactly one rotation angle is illegal. > IMO the code should not generate a value like that in the first > place. > You're right. I was thinking about this when creating this patch. But then I was not sure if "mode->rotation_reflection" is supposed to be a valid rotation value.The zero value is currently used to check if any rotation/reflection was specified at all: [...] > > 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) { I can try to make your suggested change in the cmdline parsing code, but since this sounds like a larger change I would appreciate some other opinions first. Thanks, Stephan > > > > - 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.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel
Hi Maxime, On Mon, Dec 16, 2019 at 07:08:12PM +0100, Stephan Gerhold wrote: > On Mon, Dec 16, 2019 at 07:27:25PM +0200, Ville Syrjälä wrote: > > On Mon, Dec 16, 2019 at 06:10:17PM +0100, Stephan Gerhold wrote: > > > At the moment, video mode parameters like video=540x960,reflect_x, > > > (without rotation set) are not taken into account when applying the > > > mode in drm_client_modeset.c. > > > > A rotation value without exactly one rotation angle is illegal. > > IMO the code should not generate a value like that in the first > > place. > > What do you think about Ville's comment? Should we change the command line parser to generate ROTATE_0 when no rotate option is specified? This would also require updating a few of the test cases. You added the code for parsing the rotation/reflection options, so I thought I'd ask before I start working on this. Thanks, Stephan > > You're right. I was thinking about this when creating this patch. > But then I was not sure if "mode->rotation_reflection" is supposed to be > a valid rotation value.The zero value is currently used to check > if any rotation/reflection was specified at all: > > [...] > > > 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) { > > I can try to make your suggested change in the cmdline parsing code, > but since this sounds like a larger change I would appreciate > some other opinions first. > > Thanks, > Stephan > > > > > > > - 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.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel
Hi Stephan, On Mon, Jan 13, 2020 at 05:30:39PM +0100, Stephan Gerhold wrote: > On Mon, Dec 16, 2019 at 07:08:12PM +0100, Stephan Gerhold wrote: > > On Mon, Dec 16, 2019 at 07:27:25PM +0200, Ville Syrjälä wrote: > > > On Mon, Dec 16, 2019 at 06:10:17PM +0100, Stephan Gerhold wrote: > > > > At the moment, video mode parameters like video=540x960,reflect_x, > > > > (without rotation set) are not taken into account when applying the > > > > mode in drm_client_modeset.c. > > > > > > A rotation value without exactly one rotation angle is illegal. > > > IMO the code should not generate a value like that in the first > > > place. > > > > > What do you think about Ville's comment? > Should we change the command line parser to generate ROTATE_0 when no > rotate option is specified? This would also require updating a few of > the test cases. I agree with him :) Maxime
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;
At the moment, video mode parameters like video=540x960,reflect_x, (without rotation set) are not taken into account when applying the mode in drm_client_modeset.c. 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> --- v1: https://lists.freedesktop.org/archives/dri-devel/2019-December/248145.html Changes in v2: - Clarified commit message - parameters are parsed correctly, but not taken into account when applying the mode. --- drivers/gpu/drm/drm_client_modeset.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)