| Message ID | 20220218010054.315026-5-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358767: Add DSI-to-DPI mode support | expand |
Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut: > Implement .atomic_check callback which prevents user space from setting > unsupported mode. The tc_edp_common_atomic_check() variant is already > prepared for DSI-to-DPI mode addition, which has different frequency > limits. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Maxime Ripard <maxime@cerno.tech> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > --- > V2: - New patch > --- > drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 522c2c4d8514f..01d11adee6c74 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge, > return true; > } > > +static int tc_edp_common_atomic_check(struct drm_bridge *bridge, Drop the edp in the name here? Later in the series you call this function from the DPI code, so this breaks the nice clean naming separation from patch 1. > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + const unsigned int max_khz) > +{ > + tc_bridge_mode_fixup(bridge, &crtc_state->mode, > + &crtc_state->adjusted_mode); > + > + if (crtc_state->adjusted_mode.clock > max_khz) > + crtc_state->adjusted_mode.clock = max_khz; I don't think this is correct. The adjusted most is just for minor adjustments if the bridge can not fully match the mode. If userspace supplies a invalid high modeclock I think it would be better to fail the atomic check -> return -EINVAL Regards, Lucas > + > + return 0; > +} > + > +static int tc_edp_atomic_check(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + /* DPI->(e)DP interface clock limitation: upto 154 MHz */ > + return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state, > + conn_state, 154000); > +} > + > static enum drm_mode_status > tc_edp_mode_valid(struct drm_bridge *bridge, > const struct drm_display_info *info, > @@ -1463,6 +1488,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = { > .detach = tc_edp_bridge_detach, > .mode_valid = tc_edp_mode_valid, > .mode_set = tc_bridge_mode_set, > + .atomic_check = tc_edp_atomic_check, > .atomic_enable = tc_edp_bridge_atomic_enable, > .atomic_disable = tc_edp_bridge_atomic_disable, > .mode_fixup = tc_bridge_mode_fixup,
On 2/18/22 18:34, Lucas Stach wrote: Hi [...] >> drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c >> index 522c2c4d8514f..01d11adee6c74 100644 >> --- a/drivers/gpu/drm/bridge/tc358767.c >> +++ b/drivers/gpu/drm/bridge/tc358767.c >> @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge, >> return true; >> } >> >> +static int tc_edp_common_atomic_check(struct drm_bridge *bridge, > > Drop the edp in the name here? Later in the series you call this > function from the DPI code, so this breaks the nice clean naming > separation from patch 1. > >> + struct drm_bridge_state *bridge_state, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state, >> + const unsigned int max_khz) >> +{ >> + tc_bridge_mode_fixup(bridge, &crtc_state->mode, >> + &crtc_state->adjusted_mode); >> + >> + if (crtc_state->adjusted_mode.clock > max_khz) >> + crtc_state->adjusted_mode.clock = max_khz; > > I don't think this is correct. The adjusted most is just for minor > adjustments if the bridge can not fully match the mode. If userspace > supplies a invalid high modeclock I think it would be better to fail > the atomic check -> return -EINVAL Maxime was telling me that returning -EINVAL from atomic_check is weird, so maybe we should also wait for his opinion on this part.
On Sat, Feb 19, 2022 at 03:26:40AM +0100, Marek Vasut wrote: > On 2/18/22 18:34, Lucas Stach wrote: > > Hi > > [...] > > > > drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++ > > > 1 file changed, 26 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > > > index 522c2c4d8514f..01d11adee6c74 100644 > > > --- a/drivers/gpu/drm/bridge/tc358767.c > > > +++ b/drivers/gpu/drm/bridge/tc358767.c > > > @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge, > > > return true; > > > } > > > +static int tc_edp_common_atomic_check(struct drm_bridge *bridge, > > > > Drop the edp in the name here? Later in the series you call this > > function from the DPI code, so this breaks the nice clean naming > > separation from patch 1. > > > > > + struct drm_bridge_state *bridge_state, > > > + struct drm_crtc_state *crtc_state, > > > + struct drm_connector_state *conn_state, > > > + const unsigned int max_khz) > > > +{ > > > + tc_bridge_mode_fixup(bridge, &crtc_state->mode, > > > + &crtc_state->adjusted_mode); > > > + > > > + if (crtc_state->adjusted_mode.clock > max_khz) > > > + crtc_state->adjusted_mode.clock = max_khz; > > > > I don't think this is correct. The adjusted most is just for minor > > adjustments if the bridge can not fully match the mode. If userspace > > supplies a invalid high modeclock I think it would be better to fail > > the atomic check -> return -EINVAL > > Maxime was telling me that returning -EINVAL from atomic_check is weird, so > maybe we should also wait for his opinion on this part. That was in a completely different context? Our discussion was about how you would propagate clock constraints across a pipeline, and I was telling you that it would be weird to return -EINVAL for a mode that was reported on a connector as supported (or even preferred). My argument was for mode_valid to filter them out. If your clock is way above what you can support on your device, then returning an error in atomic_check is the right thing to do. Maxime
On 2/21/22 10:01, Maxime Ripard wrote: > On Sat, Feb 19, 2022 at 03:26:40AM +0100, Marek Vasut wrote: >> On 2/18/22 18:34, Lucas Stach wrote: >> >> Hi >> >> [...] >> >>>> drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c >>>> index 522c2c4d8514f..01d11adee6c74 100644 >>>> --- a/drivers/gpu/drm/bridge/tc358767.c >>>> +++ b/drivers/gpu/drm/bridge/tc358767.c >>>> @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge, >>>> return true; >>>> } >>>> +static int tc_edp_common_atomic_check(struct drm_bridge *bridge, >>> >>> Drop the edp in the name here? Later in the series you call this >>> function from the DPI code, so this breaks the nice clean naming >>> separation from patch 1. >>> >>>> + struct drm_bridge_state *bridge_state, >>>> + struct drm_crtc_state *crtc_state, >>>> + struct drm_connector_state *conn_state, >>>> + const unsigned int max_khz) >>>> +{ >>>> + tc_bridge_mode_fixup(bridge, &crtc_state->mode, >>>> + &crtc_state->adjusted_mode); >>>> + >>>> + if (crtc_state->adjusted_mode.clock > max_khz) >>>> + crtc_state->adjusted_mode.clock = max_khz; >>> >>> I don't think this is correct. The adjusted most is just for minor >>> adjustments if the bridge can not fully match the mode. If userspace >>> supplies a invalid high modeclock I think it would be better to fail >>> the atomic check -> return -EINVAL >> >> Maxime was telling me that returning -EINVAL from atomic_check is weird, so >> maybe we should also wait for his opinion on this part. > > That was in a completely different context? > > Our discussion was about how you would propagate clock constraints > across a pipeline, and I was telling you that it would be weird to > return -EINVAL for a mode that was reported on a connector as supported > (or even preferred). > > My argument was for mode_valid to filter them out. > > If your clock is way above what you can support on your device, then > returning an error in atomic_check is the right thing to do. Ah OK
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 522c2c4d8514f..01d11adee6c74 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge, return true; } +static int tc_edp_common_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + const unsigned int max_khz) +{ + tc_bridge_mode_fixup(bridge, &crtc_state->mode, + &crtc_state->adjusted_mode); + + if (crtc_state->adjusted_mode.clock > max_khz) + crtc_state->adjusted_mode.clock = max_khz; + + return 0; +} + +static int tc_edp_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + /* DPI->(e)DP interface clock limitation: upto 154 MHz */ + return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state, + conn_state, 154000); +} + static enum drm_mode_status tc_edp_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info, @@ -1463,6 +1488,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = { .detach = tc_edp_bridge_detach, .mode_valid = tc_edp_mode_valid, .mode_set = tc_bridge_mode_set, + .atomic_check = tc_edp_atomic_check, .atomic_enable = tc_edp_bridge_atomic_enable, .atomic_disable = tc_edp_bridge_atomic_disable, .mode_fixup = tc_bridge_mode_fixup,
Implement .atomic_check callback which prevents user space from setting unsupported mode. The tc_edp_common_atomic_check() variant is already prepared for DSI-to-DPI mode addition, which has different frequency limits. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Maxime Ripard <maxime@cerno.tech> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Sam Ravnborg <sam@ravnborg.org> --- V2: - New patch --- drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)