[PATCH V2 04/11] drm/bridge: tc358767: Implement atomic_check callback

[Maxime Ripard]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220221/32e83d28/attachment.sig>