From: Dmitry Osipenko <digetx@gmail.com>
To: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Thierry Reding <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Sean Paul <sean@poorly.run>, Daniel Vetter <daniel@ffwll.ch>,
David Airlie <airlied@linux.ie>,
dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] drm/modes: Skip invalid cmdline mode
Date: Wed, 10 Jul 2019 19:03:03 +0300 [thread overview]
Message-ID: <5b6bc04d-391e-3370-ffe5-17094ae4b365@gmail.com> (raw)
In-Reply-To: <2b17baa6-0d16-acac-f626-51799f0d3293@gmail.com>
10.07.2019 18:45, Dmitry Osipenko пишет:
> 10.07.2019 18:05, Dmitry Osipenko пишет:
>> 10.07.2019 17:05, Maxime Ripard пишет:
>>> On Wed, Jul 10, 2019 at 04:29:19PM +0300, Dmitry Osipenko wrote:
>>>> This works:
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>>>> index 56d36779d213..e5a2f9c8f404 100644
>>>> --- a/drivers/gpu/drm/drm_client_modeset.c
>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>>>> @@ -182,6 +182,8 @@ drm_connector_pick_cmdline_mode(struct drm_connector *connector)
>>>> mode = drm_mode_create_from_cmdline_mode(connector->dev, cmdline_mode);
>>>> if (mode)
>>>> list_add(&mode->head, &connector->modes);
>>>> + else
>>>> + cmdline_mode->specified = false;
>>>
>>> Hmmm, it's not clear to me why that wouldn't be the case.
>>>
>>> If we come back to the beginning of that function, we retrieve the
>>> cmdline_mode buffer from the connector pointer, that will probably
>>> have been parsed a first time using drm_mode_create_from_cmdline_mode
>>> in drm_helper_probe_add_cmdline_mode.
>>>
>>> Now, I'm guessing that the issue is that in
>>> drm_mode_parse_command_line_for_connector, if we have a named mode, we
>>> just copy the mode over and set mode->specified.
>>>
>>> And we then move over to do other checks, and that's probably what
>>> fails and returns, but our drm_cmdline_mode will have been modified.
>>>
>>> I'm not entirely sure how to deal with that though.
>>>
>>> I guess we could allocate a drm_cmdline_mode structure on the stack,
>>> fill that, and if successful copy over its content to the one in
>>> drm_connector. That would allow us to only change the content on
>>> success, which is what I would expect from such a function?
>>>
>>> How does that sound?
>>
>> I now see that there is DRM_MODE_TYPE_USERDEF flag that is assigned only
>> for the "cmdline" mode and drm_client_rotation() is the only place in
>> DRM code that cares about whether mode is from cmdline, hence looks like
>> it will be more correct to do the following:
>>
>> diff --git a/drivers/gpu/drm/drm_client_modeset.c
>> b/drivers/gpu/drm/drm_client_modeset.c
>> index 56d36779d213..e5b3be9ed689 100644
>> --- a/drivers/gpu/drm/drm_client_modeset.c
>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> @@ -825,6 +825,7 @@ bool drm_client_rotation(struct drm_mode_set
>> *modeset, unsigned int *rotation)
>> {
>> struct drm_connector *connector = modeset->connectors[0];
>> struct drm_plane *plane = modeset->crtc->primary;
>> + struct drm_display_mode *mode = modeset->mode;
>> struct drm_cmdline_mode *cmdline;
>> u64 valid_mask = 0;
>> unsigned int i;
>> @@ -859,7 +860,7 @@ bool drm_client_rotation(struct drm_mode_set
>> *modeset, unsigned int *rotation)
>> * simple XOR between the two handle the addition nicely.
>> */
>> cmdline = &connector->cmdline_mode;
>> - if (cmdline->specified) {
>> + if (mode->flags & DRM_MODE_TYPE_USERDEF) {
>> unsigned int cmdline_rest, panel_rest;
>> unsigned int cmdline_rot, panel_rot;
>> unsigned int sum_rot, sum_rest;
>>
>
> Although, then rotation won't be applied to the named mode in that case.
>
> Seems the fix could be even simpler:
>
> @@ -859,7 +859,7 @@ bool drm_client_rotation(struct drm_mode_set
> *modeset, unsigned int *rotation)
> * simple XOR between the two handle the addition nicely.
> */
> cmdline = &connector->cmdline_mode;
> - if (cmdline->specified) {
> + if (cmdline->specified && cmdline->rotation_reflection) {
> unsigned int cmdline_rest, panel_rest;
> unsigned int cmdline_rot, panel_rot;
> unsigned int sum_rot, sum_rest;
>
> And looks like there is another problem here.. the cmdline's rotation
> overrides *all* modes while the doc/fb/modedb.rst claims that rotation
> is applied only to the *initial* mode.
>
Actually, I was wrong here. This rotation is getting applied only to the
framebuffer's console, so looks fine.
I guess it's also okay to just keep applying cmdline's rotation even if
the mode is invalid. Please let me know if you have any objections.
next prev parent reply other threads:[~2019-07-10 16:03 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-09 14:51 [PATCH v1] drm/modes: Skip invalid cmdline mode Dmitry Osipenko
2019-07-10 10:00 ` Ville Syrjälä
2019-07-10 14:03 ` Dmitry Osipenko
2019-07-10 10:12 ` Maxime Ripard
2019-07-10 12:42 ` Dmitry Osipenko
2019-07-10 12:55 ` Maxime Ripard
2019-07-10 12:59 ` Dmitry Osipenko
2019-07-10 13:06 ` Maxime Ripard
2019-07-10 13:11 ` Dmitry Osipenko
2019-07-10 13:29 ` Dmitry Osipenko
2019-07-10 13:36 ` Dmitry Osipenko
2019-07-10 14:05 ` Maxime Ripard
2019-07-10 15:05 ` Dmitry Osipenko
2019-07-10 15:45 ` Dmitry Osipenko
2019-07-10 16:03 ` Dmitry Osipenko [this message]
2019-07-11 9:03 ` Maxime Ripard
2019-07-11 15:55 ` Dmitry Osipenko
2019-07-12 8:10 ` Maxime Ripard
2019-07-12 8:30 ` Dmitry Osipenko
2019-07-12 19:53 ` Maxime Ripard
2019-07-13 16:41 ` Dmitry Osipenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5b6bc04d-391e-3370-ffe5-17094ae4b365@gmail.com \
--to=digetx@gmail.com \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=jonathanh@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=maxime.ripard@bootlin.com \
--cc=sean@poorly.run \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).