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
signature.asc
Description: PGP signature
