Re: [PATCH v2 3/5] drm/modes: Allow to specify rotation and reflection on the commandline

Noralf Trønnes
2019-04-17T14:58:30Z

Den 17.04.2019 16.30, skrev Maxime Ripard:
> Hi Noralf,
>
> On Tue, Apr 16, 2019 at 04:50:00PM +0200, Noralf Trønnes wrote:
>> Den 11.04.2019 15.22, skrev Maxime Ripard:
>>> Rotations and reflections setup are needed in some scenarios to initialise
>>> properly the initial framebuffer. Some drivers already had a bunch of
>>> quirks to deal with this, such as either a private kernel command line
>>> parameter (omapdss) or on the device tree (various panels).
>>>
>>> In order to accomodate this, let's create a video mode parameter to deal
>>> with the rotation and reflexion.
>>>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>> ---
>>> drivers/gpu/drm/drm_fb_helper.c | 4 +-
>>> drivers/gpu/drm/drm_modes.c | 110 +++++++++++++++++++++++++++------
>>> include/drm/drm_connector.h | 1 +-
>>> 3 files changed, 95 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index b3a5d79436ae..8781897559b2 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -2521,6 +2521,7 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>>> struct drm_connector *connector)
>>> {
>>> struct drm_plane *plane = fb_crtc->mode_set.crtc->primary;
>>> + struct drm_cmdline_mode *mode = &connector->cmdline_mode;
>>> uint64_t valid_mask = 0;
>>> int i, rotation;
>>>
>>> @@ -2540,6 +2541,9 @@ static void drm_setup_crtc_rotation(struct drm_fb_helper *fb_helper,
>>> rotation = DRM_MODE_ROTATE_0;
>>> }
>>>
>>> + if (mode->rotation != DRM_MODE_ROTATE_0)
>>> + fb_crtc->rotation = mode->rotation;
>>> +
>>
>> We already have a property to describe initial display/panel rotation.
>> If we can set connector->display_info.panel_orientation from the video=
>> parameter, then there's no need to modify drm_fb_helper, it will just work.
>>
>> In that case, maybe 'orientation' is a better argument name with values
>> mapped to the enum.
>
> I wouldn't put it at the same level though. As far as I understand it,
> the orientation is a hardware constraint: the hardware has been
> designed that way, and should honor that orientation to make it look
> with the top, well, on top.
>
> However, the rotation is more of a user choice, and you could
> definitely envision having a combination of a rotation and an
> orientation constraint.
>

Does this rotation only apply to the fbdev emulation and should not
propogate to DRM userspace?

I actually don't understand how the DRM plane rotation works for 90/270
rotation. Should the framebuffer width/height be swapped when attaching
to a rotated plane?

Btw drm_setup_crtc_rotation() doesn't support 90/270 plane rotation even
if the hw supports it. It will instead sw rotate fbcon, but fbdev
userspace remains unrotated.

Noralf.

> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com