| Message ID | 20191110154101.26486-13-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/modes: parse_cmdline: Add support for specifying panel_orientation on the kernel cmdline | expand |
On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote: > If the new video=... panel_orientation option is set for a connector, honor > it and setup a matching "panel orientation" property on the connector. > > BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Don't we need one more patch here to create the panel orientation property if the driver doesn't do it? Otherwise this won't happen, and userspace can't look at it (only the internal fbdev stuff, which has it's own limitations wrt rotation). -Daniel > --- > drivers/gpu/drm/drm_connector.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 40a985c411a6..591914f01cd4 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) > connector->force = mode->force; > } > > + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { > + DRM_INFO("setting connector %s panel_orientation to %d\n", > + connector->name, mode->panel_orientation); > + drm_connector_set_panel_orientation(connector, > + mode->panel_orientation); > + } > + > DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", > connector->name, mode->name, > mode->xres, mode->yres, > -- > 2.23.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, On 12-11-2019 10:47, Daniel Vetter wrote: > On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote: >> If the new video=... panel_orientation option is set for a connector, honor >> it and setup a matching "panel orientation" property on the connector. >> >> BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Don't we need one more patch here to create the panel orientation property > if the driver doesn't do it? Otherwise this won't happen, and userspace > can't look at it (only the internal fbdev stuff, which has it's own > limitations wrt rotation). That is what patch 11/12 is for, that patch renames drm_connector_init_panel_orientation to drm_set_panel_orientation and makes it both set, init and attach the property (unless called with DRM_MODE_PANEL_ORIENTATION_UNKNOWN in which case it is a no-op). Patch 11/12 also makes drm_set_panel_orientation do the set only once, which is why the orientation to set is now passed instead of stored directly in the connector, so that a second set call (panel_orientation of the connector already != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) can be skipped, so that the cmdline overrides driver detecion code for this. Regards, Hans > -Daniel > >> --- >> drivers/gpu/drm/drm_connector.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >> index 40a985c411a6..591914f01cd4 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) >> connector->force = mode->force; >> } >> >> + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { >> + DRM_INFO("setting connector %s panel_orientation to %d\n", >> + connector->name, mode->panel_orientation); >> + drm_connector_set_panel_orientation(connector, >> + mode->panel_orientation); >> + } >> + >> DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", >> connector->name, mode->name, >> mode->xres, mode->yres, >> -- >> 2.23.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, Nov 12, 2019 at 11:43 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 12-11-2019 10:47, Daniel Vetter wrote: > > On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote: > >> If the new video=... panel_orientation option is set for a connector, honor > >> it and setup a matching "panel orientation" property on the connector. > >> > >> BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Don't we need one more patch here to create the panel orientation property > > if the driver doesn't do it? Otherwise this won't happen, and userspace > > can't look at it (only the internal fbdev stuff, which has it's own > > limitations wrt rotation). > > That is what patch 11/12 is for, that patch renames drm_connector_init_panel_orientation > to drm_set_panel_orientation and makes it both set, init and attach the property > (unless called with DRM_MODE_PANEL_ORIENTATION_UNKNOWN in which case it is a no-op). > > Patch 11/12 also makes drm_set_panel_orientation do the set only once, which is why > the orientation to set is now passed instead of stored directly in the connector, > so that a second set call (panel_orientation of the connector already != > DRM_MODE_PANEL_ORIENTATION_UNKNOWN) can be skipped, so that the cmdline overrides > driver detecion code for this. Oh, that's what I get for not reading the entire series ... Only risk with that is that drivers set this property after driver loading is done - we can't attach/create properties after drm_dev_register. But I've added the corresponding WARN_ON to these, so we should be all fine I think. So looks all good to me. -Daniel > > Regards, > > Hans > > > > > -Daniel > > > >> --- > >> drivers/gpu/drm/drm_connector.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > >> index 40a985c411a6..591914f01cd4 100644 > >> --- a/drivers/gpu/drm/drm_connector.c > >> +++ b/drivers/gpu/drm/drm_connector.c > >> @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) > >> connector->force = mode->force; > >> } > >> > >> + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { > >> + DRM_INFO("setting connector %s panel_orientation to %d\n", > >> + connector->name, mode->panel_orientation); > >> + drm_connector_set_panel_orientation(connector, > >> + mode->panel_orientation); > >> + } > >> + > >> DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", > >> connector->name, mode->name, > >> mode->xres, mode->yres, > >> -- > >> 2.23.0 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
Hi, On 12-11-2019 14:32, Daniel Vetter wrote: > On Tue, Nov 12, 2019 at 11:43 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 12-11-2019 10:47, Daniel Vetter wrote: >>> On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote: >>>> If the new video=... panel_orientation option is set for a connector, honor >>>> it and setup a matching "panel orientation" property on the connector. >>>> >>>> BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> >>> Don't we need one more patch here to create the panel orientation property >>> if the driver doesn't do it? Otherwise this won't happen, and userspace >>> can't look at it (only the internal fbdev stuff, which has it's own >>> limitations wrt rotation). >> >> That is what patch 11/12 is for, that patch renames drm_connector_init_panel_orientation >> to drm_set_panel_orientation and makes it both set, init and attach the property >> (unless called with DRM_MODE_PANEL_ORIENTATION_UNKNOWN in which case it is a no-op). >> >> Patch 11/12 also makes drm_set_panel_orientation do the set only once, which is why >> the orientation to set is now passed instead of stored directly in the connector, >> so that a second set call (panel_orientation of the connector already != >> DRM_MODE_PANEL_ORIENTATION_UNKNOWN) can be skipped, so that the cmdline overrides >> driver detecion code for this. > > Oh, that's what I get for not reading the entire series ... Only risk > with that is that drivers set this property after driver loading is > done - we can't attach/create properties after drm_dev_register. But > I've added the corresponding WARN_ON to these, so we should be all > fine I think. So looks all good to me. Can I take that as your Acked-by for this patch and perhaps also for 11/12 ? Also what about your remarks to 10/12? I'm happy to do a v2 with a memset, as said my main reason for dropping the specified=false in the early path is that we never init bpp_specified or refresh_specified explicitly to false I'm all for being explicit about that, but then lets just zero out the entire passed in drm_cmdline_mode struct. Regards, Hans >>>> --- >>>> drivers/gpu/drm/drm_connector.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>>> index 40a985c411a6..591914f01cd4 100644 >>>> --- a/drivers/gpu/drm/drm_connector.c >>>> +++ b/drivers/gpu/drm/drm_connector.c >>>> @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) >>>> connector->force = mode->force; >>>> } >>>> >>>> + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { >>>> + DRM_INFO("setting connector %s panel_orientation to %d\n", >>>> + connector->name, mode->panel_orientation); >>>> + drm_connector_set_panel_orientation(connector, >>>> + mode->panel_orientation); >>>> + } >>>> + >>>> DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", >>>> connector->name, mode->name, >>>> mode->xres, mode->yres, >>>> -- >>>> 2.23.0 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >> > >
On Tue, Nov 12, 2019 at 2:39 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 12-11-2019 14:32, Daniel Vetter wrote: > > On Tue, Nov 12, 2019 at 11:43 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi, > >> > >> On 12-11-2019 10:47, Daniel Vetter wrote: > >>> On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote: > >>>> If the new video=... panel_orientation option is set for a connector, honor > >>>> it and setup a matching "panel orientation" property on the connector. > >>>> > >>>> BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 > >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>> > >>> Don't we need one more patch here to create the panel orientation property > >>> if the driver doesn't do it? Otherwise this won't happen, and userspace > >>> can't look at it (only the internal fbdev stuff, which has it's own > >>> limitations wrt rotation). > >> > >> That is what patch 11/12 is for, that patch renames drm_connector_init_panel_orientation > >> to drm_set_panel_orientation and makes it both set, init and attach the property > >> (unless called with DRM_MODE_PANEL_ORIENTATION_UNKNOWN in which case it is a no-op). > >> > >> Patch 11/12 also makes drm_set_panel_orientation do the set only once, which is why > >> the orientation to set is now passed instead of stored directly in the connector, > >> so that a second set call (panel_orientation of the connector already != > >> DRM_MODE_PANEL_ORIENTATION_UNKNOWN) can be skipped, so that the cmdline overrides > >> driver detecion code for this. > > > > Oh, that's what I get for not reading the entire series ... Only risk > > with that is that drivers set this property after driver loading is > > done - we can't attach/create properties after drm_dev_register. But > > I've added the corresponding WARN_ON to these, so we should be all > > fine I think. So looks all good to me. > > Can I take that as your Acked-by for this patch and perhaps also for 11/12 ? Uh I didn't really read the details, ack feels a bit thin to land this ... > Also what about your remarks to 10/12? I'm happy to do a v2 with a memset, > as said my main reason for dropping the specified=false in the early path > is that we never init bpp_specified or refresh_specified explicitly to false > I'm all for being explicit about that, but then lets just zero out the entire > passed in drm_cmdline_mode struct. Hm yeah, clearing it all might be a good idea. -Daniel > > Regards, > > Hans > > > > >>>> --- > >>>> drivers/gpu/drm/drm_connector.c | 7 +++++++ > >>>> 1 file changed, 7 insertions(+) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > >>>> index 40a985c411a6..591914f01cd4 100644 > >>>> --- a/drivers/gpu/drm/drm_connector.c > >>>> +++ b/drivers/gpu/drm/drm_connector.c > >>>> @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) > >>>> connector->force = mode->force; > >>>> } > >>>> > >>>> + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { > >>>> + DRM_INFO("setting connector %s panel_orientation to %d\n", > >>>> + connector->name, mode->panel_orientation); > >>>> + drm_connector_set_panel_orientation(connector, > >>>> + mode->panel_orientation); > >>>> + } > >>>> + > >>>> DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", > >>>> connector->name, mode->name, > >>>> mode->xres, mode->yres, > >>>> -- > >>>> 2.23.0 > >>>> > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>> > >> > > > > >
Hi, On 12-11-2019 14:47, Daniel Vetter wrote: > On Tue, Nov 12, 2019 at 2:39 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 12-11-2019 14:32, Daniel Vetter wrote: >>> On Tue, Nov 12, 2019 at 11:43 AM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi, >>>> >>>> On 12-11-2019 10:47, Daniel Vetter wrote: >>>>> On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote: >>>>>> If the new video=... panel_orientation option is set for a connector, honor >>>>>> it and setup a matching "panel orientation" property on the connector. >>>>>> >>>>>> BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> >>>>> Don't we need one more patch here to create the panel orientation property >>>>> if the driver doesn't do it? Otherwise this won't happen, and userspace >>>>> can't look at it (only the internal fbdev stuff, which has it's own >>>>> limitations wrt rotation). >>>> >>>> That is what patch 11/12 is for, that patch renames drm_connector_init_panel_orientation >>>> to drm_set_panel_orientation and makes it both set, init and attach the property >>>> (unless called with DRM_MODE_PANEL_ORIENTATION_UNKNOWN in which case it is a no-op). >>>> >>>> Patch 11/12 also makes drm_set_panel_orientation do the set only once, which is why >>>> the orientation to set is now passed instead of stored directly in the connector, >>>> so that a second set call (panel_orientation of the connector already != >>>> DRM_MODE_PANEL_ORIENTATION_UNKNOWN) can be skipped, so that the cmdline overrides >>>> driver detecion code for this. >>> >>> Oh, that's what I get for not reading the entire series ... Only risk >>> with that is that drivers set this property after driver loading is >>> done - we can't attach/create properties after drm_dev_register. But >>> I've added the corresponding WARN_ON to these, so we should be all >>> fine I think. So looks all good to me. >> >> Can I take that as your Acked-by for this patch and perhaps also for 11/12 ? > > Uh I didn't really read the details, ack feels a bit thin to land this ... Ok, I will wait a bit for others to review this then. Note that Maxime has reviewed patches 1-9, with one small remark on patch 9 which I'm still awaiting an answer for. So most of the cmdline parsing stuff has been reviewed and if you are ok with the non cmdline parsing changes... >> Also what about your remarks to 10/12? I'm happy to do a v2 with a memset, >> as said my main reason for dropping the specified=false in the early path >> is that we never init bpp_specified or refresh_specified explicitly to false >> I'm all for being explicit about that, but then lets just zero out the entire >> passed in drm_cmdline_mode struct. > > Hm yeah, clearing it all might be a good idea. Ok I will submit a v2 with this change. Regards, Hans >>>>>> --- >>>>>> drivers/gpu/drm/drm_connector.c | 7 +++++++ >>>>>> 1 file changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >>>>>> index 40a985c411a6..591914f01cd4 100644 >>>>>> --- a/drivers/gpu/drm/drm_connector.c >>>>>> +++ b/drivers/gpu/drm/drm_connector.c >>>>>> @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) >>>>>> connector->force = mode->force; >>>>>> } >>>>>> >>>>>> + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { >>>>>> + DRM_INFO("setting connector %s panel_orientation to %d\n", >>>>>> + connector->name, mode->panel_orientation); >>>>>> + drm_connector_set_panel_orientation(connector, >>>>>> + mode->panel_orientation); >>>>>> + } >>>>>> + >>>>>> DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", >>>>>> connector->name, mode->name, >>>>>> mode->xres, mode->yres, >>>>>> -- >>>>>> 2.23.0 >>>>>> >>>>>> _______________________________________________ >>>>>> dri-devel mailing list >>>>>> dri-devel@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>> >>>> >>> >>> >> > >
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 40a985c411a6..591914f01cd4 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector) connector->force = mode->force; } + if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) { + DRM_INFO("setting connector %s panel_orientation to %d\n", + connector->name, mode->panel_orientation); + drm_connector_set_panel_orientation(connector, + mode->panel_orientation); + } + DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n", connector->name, mode->name, mode->xres, mode->yres,
If the new video=... panel_orientation option is set for a connector, honor it and setup a matching "panel orientation" property on the connector. BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83 Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/drm_connector.c | 7 +++++++ 1 file changed, 7 insertions(+)