* [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper @ 2017-04-07 12:17 Romain Perier 2017-04-07 17:15 ` Archit Taneja 2017-04-07 18:33 ` Daniel Vetter 0 siblings, 2 replies; 5+ messages in thread From: Romain Perier @ 2017-04-07 12:17 UTC (permalink / raw) To: Archit Taneja, David Airlie Cc: dri-devel, linux-rockchip, linux-arm-kernel, linux-kernel, Romain Perier This helper is supposed to validate or reject the modeline before it applied by the mode setting. Currently this function has been dropped, it was previously set to a dummy function that always returned true. For both cases, this means that userspace can ask for a bad modeline that will be always accepted. On some platforms, like Rockchip, the drm dw_hdmi-rockchip variant driver already implements the atomic_check drm helper, so mode_fixup cannot be handled and implemented there (as drm_atomic_helper relies on either atomic_check or mode_fixup). This commit implements this helper. It only checks that this mode is correct from the connector point of view Signed-off-by: Romain Perier <romain.perier@collabora.com> --- drivers/gpu/drm/bridge/dw-hdmi.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 22211ff..3bd0807 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1740,6 +1740,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) return 0; } + +static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge, + const struct drm_display_mode *orig_mode, + struct drm_display_mode *mode) +{ + struct dw_hdmi *hdmi = bridge->driver_private; + struct drm_connector *connector = &hdmi->connector; + enum drm_mode_status status; + + status = dw_hdmi_connector_mode_valid(connector, mode); + if (status != MODE_OK) + return false; + return true; +} + static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, struct drm_display_mode *orig_mode, struct drm_display_mode *mode) @@ -1781,6 +1796,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set, + .mode_fixup = dw_hdmi_bridge_mode_fixup, }; static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) -- 2.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper 2017-04-07 12:17 [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper Romain Perier @ 2017-04-07 17:15 ` Archit Taneja 2017-04-07 18:31 ` Daniel Vetter 2017-04-07 18:33 ` Daniel Vetter 1 sibling, 1 reply; 5+ messages in thread From: Archit Taneja @ 2017-04-07 17:15 UTC (permalink / raw) To: Romain Perier, daniel.vetter Cc: David Airlie, linux-rockchip, linux-arm-kernel, dri-devel, linux-kernel Hi, On 4/7/2017 5:47 PM, Romain Perier wrote: > This helper is supposed to validate or reject the modeline before it > applied by the mode setting. Currently this function has been dropped, > it was previously set to a dummy function that always returned true. For > both cases, this means that userspace can ask for a bad modeline that > will be always accepted. > > On some platforms, like Rockchip, the drm dw_hdmi-rockchip variant driver > already implements the atomic_check drm helper, so mode_fixup cannot be > handled and implemented there (as drm_atomic_helper relies on either > atomic_check or mode_fixup). > > This commit implements this helper. It only checks that this mode is > correct from the connector point of view We do have a atomic_check op in drm_connector_helper_funcs. I've rarely seen it being used, but it could be used to validate the mode w.r.t the connector, rather than checking it in the bridge's mode_fixup op. Daniel, Is it okay to use the connector's atomic_check to validate a mode. (by peeping into the new_crtc_state->mode?) Thanks, Archit > > Signed-off-by: Romain Perier <romain.perier@collabora.com> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c > index 22211ff..3bd0807 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1740,6 +1740,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > return 0; > } > > + > +static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *orig_mode, > + struct drm_display_mode *mode) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + struct drm_connector *connector = &hdmi->connector; > + enum drm_mode_status status; > + > + status = dw_hdmi_connector_mode_valid(connector, mode); > + if (status != MODE_OK) > + return false; > + return true; > +} > + > static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, > struct drm_display_mode *orig_mode, > struct drm_display_mode *mode) > @@ -1781,6 +1796,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > .enable = dw_hdmi_bridge_enable, > .disable = dw_hdmi_bridge_disable, > .mode_set = dw_hdmi_bridge_mode_set, > + .mode_fixup = dw_hdmi_bridge_mode_fixup, > }; > > static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper 2017-04-07 17:15 ` Archit Taneja @ 2017-04-07 18:31 ` Daniel Vetter 0 siblings, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2017-04-07 18:31 UTC (permalink / raw) To: Archit Taneja Cc: Romain Perier, David Airlie, open list:ARM/Rockchip SoC..., linux-arm-kernel, dri-devel, Linux Kernel Mailing List On Fri, Apr 7, 2017 at 7:15 PM, Archit Taneja <architt@codeaurora.org> wrote: > On 4/7/2017 5:47 PM, Romain Perier wrote: >> >> This helper is supposed to validate or reject the modeline before it >> applied by the mode setting. Currently this function has been dropped, >> it was previously set to a dummy function that always returned true. For >> both cases, this means that userspace can ask for a bad modeline that >> will be always accepted. >> >> On some platforms, like Rockchip, the drm dw_hdmi-rockchip variant driver >> already implements the atomic_check drm helper, so mode_fixup cannot be >> handled and implemented there (as drm_atomic_helper relies on either >> atomic_check or mode_fixup). >> >> This commit implements this helper. It only checks that this mode is >> correct from the connector point of view > > > We do have a atomic_check op in drm_connector_helper_funcs. I've > rarely seen it being used, but it could be used to validate > the mode w.r.t the connector, rather than checking it in the > bridge's mode_fixup op. > > Daniel, > > Is it okay to use the connector's atomic_check to validate a mode. > (by peeping into the new_crtc_state->mode?) This new atomic_check is super-new, and only for validating properties on the connector, before stuff like the routing or crtc is all set up. So no, can't be used for that. mode_fixup on the bridge is indeed the right place for limiting output specific stuff. Yes this is all a bit awkward, but thus far no one volunteered to improve the helpers after I explained the full nastiness of the situation ... I think we should have some helpers that implement the mode_valid helper in terms of the mode_fixup/atomic_check functions, but that's a bit tricky to get right :( -Daniel > > Thanks, > Archit > > >> >> Signed-off-by: Romain Perier <romain.perier@collabora.com> >> --- >> drivers/gpu/drm/bridge/dw-hdmi.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c >> b/drivers/gpu/drm/bridge/dw-hdmi.c >> index 22211ff..3bd0807 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -1740,6 +1740,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge >> *bridge) >> return 0; >> } >> >> + >> +static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge, >> + const struct drm_display_mode >> *orig_mode, >> + struct drm_display_mode *mode) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + struct drm_connector *connector = &hdmi->connector; >> + enum drm_mode_status status; >> + >> + status = dw_hdmi_connector_mode_valid(connector, mode); >> + if (status != MODE_OK) >> + return false; >> + return true; >> +} >> + >> static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, >> struct drm_display_mode *orig_mode, >> struct drm_display_mode *mode) >> @@ -1781,6 +1796,7 @@ static const struct drm_bridge_funcs >> dw_hdmi_bridge_funcs = { >> .enable = dw_hdmi_bridge_enable, >> .disable = dw_hdmi_bridge_disable, >> .mode_set = dw_hdmi_bridge_mode_set, >> + .mode_fixup = dw_hdmi_bridge_mode_fixup, >> }; >> >> static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) >> > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper 2017-04-07 12:17 [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper Romain Perier 2017-04-07 17:15 ` Archit Taneja @ 2017-04-07 18:33 ` Daniel Vetter 2017-04-10 10:54 ` Archit Taneja 1 sibling, 1 reply; 5+ messages in thread From: Daniel Vetter @ 2017-04-07 18:33 UTC (permalink / raw) To: Romain Perier Cc: Archit Taneja, David Airlie, linux-rockchip, linux-arm-kernel, dri-devel, linux-kernel On Fri, Apr 07, 2017 at 02:17:43PM +0200, Romain Perier wrote: > This helper is supposed to validate or reject the modeline before it > applied by the mode setting. Currently this function has been dropped, > it was previously set to a dummy function that always returned true. For > both cases, this means that userspace can ask for a bad modeline that > will be always accepted. > > On some platforms, like Rockchip, the drm dw_hdmi-rockchip variant driver > already implements the atomic_check drm helper, so mode_fixup cannot be > handled and implemented there (as drm_atomic_helper relies on either > atomic_check or mode_fixup). > > This commit implements this helper. It only checks that this mode is > correct from the connector point of view > > Signed-off-by: Romain Perier <romain.perier@collabora.com> Yup this is how it's supposed to be done, check bridge limits in the bridge's mode_fixup hook. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c > index 22211ff..3bd0807 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1740,6 +1740,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > return 0; > } > > + > +static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *orig_mode, > + struct drm_display_mode *mode) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + struct drm_connector *connector = &hdmi->connector; > + enum drm_mode_status status; > + > + status = dw_hdmi_connector_mode_valid(connector, mode); > + if (status != MODE_OK) > + return false; > + return true; > +} > + > static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, > struct drm_display_mode *orig_mode, > struct drm_display_mode *mode) > @@ -1781,6 +1796,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > .enable = dw_hdmi_bridge_enable, > .disable = dw_hdmi_bridge_disable, > .mode_set = dw_hdmi_bridge_mode_set, > + .mode_fixup = dw_hdmi_bridge_mode_fixup, > }; > > static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) > -- > 2.9.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper 2017-04-07 18:33 ` Daniel Vetter @ 2017-04-10 10:54 ` Archit Taneja 0 siblings, 0 replies; 5+ messages in thread From: Archit Taneja @ 2017-04-10 10:54 UTC (permalink / raw) To: Romain Perier, David Airlie, linux-rockchip, linux-arm-kernel, dri-devel, linux-kernel On 04/08/2017 12:03 AM, Daniel Vetter wrote: > On Fri, Apr 07, 2017 at 02:17:43PM +0200, Romain Perier wrote: >> This helper is supposed to validate or reject the modeline before it >> applied by the mode setting. Currently this function has been dropped, >> it was previously set to a dummy function that always returned true. For >> both cases, this means that userspace can ask for a bad modeline that >> will be always accepted. >> >> On some platforms, like Rockchip, the drm dw_hdmi-rockchip variant driver >> already implements the atomic_check drm helper, so mode_fixup cannot be >> handled and implemented there (as drm_atomic_helper relies on either >> atomic_check or mode_fixup). >> >> This commit implements this helper. It only checks that this mode is >> correct from the connector point of view >> >> Signed-off-by: Romain Perier <romain.perier@collabora.com> > > Yup this is how it's supposed to be done, check bridge limits in the > bridge's mode_fixup hook. > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> queued to drm-misc-next-fixes (so that it makes it into 4.12). Thanks, Archit >> --- >> drivers/gpu/drm/bridge/dw-hdmi.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c >> index 22211ff..3bd0807 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -1740,6 +1740,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >> return 0; >> } >> >> + >> +static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge, >> + const struct drm_display_mode *orig_mode, >> + struct drm_display_mode *mode) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + struct drm_connector *connector = &hdmi->connector; >> + enum drm_mode_status status; >> + >> + status = dw_hdmi_connector_mode_valid(connector, mode); >> + if (status != MODE_OK) >> + return false; >> + return true; >> +} >> + >> static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, >> struct drm_display_mode *orig_mode, >> struct drm_display_mode *mode) >> @@ -1781,6 +1796,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { >> .enable = dw_hdmi_bridge_enable, >> .disable = dw_hdmi_bridge_disable, >> .mode_set = dw_hdmi_bridge_mode_set, >> + .mode_fixup = dw_hdmi_bridge_mode_fixup, >> }; >> >> static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) >> -- >> 2.9.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-10 10:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-07 12:17 [PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper Romain Perier 2017-04-07 17:15 ` Archit Taneja 2017-04-07 18:31 ` Daniel Vetter 2017-04-07 18:33 ` Daniel Vetter 2017-04-10 10:54 ` Archit Taneja
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).