Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge

[Tomi Valkeinen]

Hi,

On 04/09/2020 05:29, Laurent Pinchart wrote:

>> Laurent mentioned that atomic_check should not change state. Note that
>> cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, vs and line_thresh.
> 
> .atomic_check() isn't allowed to change any global state, which means
> both hardware state and data in cdns_mhdp_device. The drm_bridge_state
> (and thus the cdns_mhdp_bridge_state) can be modified as it stores the
> state for the atomic commit being checked.
> 
>> There seems to be issues with mode changes, but I think the first step would be to clarify the
>> related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it should be renamed to
>> calculate_tu or something like that.
>>
>> cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as currently it digs that up
>> from the current state.
>>
>> Probably cdns_mhdp_validate_mode_params() would be better if it doesn't write the result to the
>> state, but returns the values. That way it could also be used to verify if suitable settings can be
>> found, without changing the state.
> 
> This use case is actually a very good example of proper usage of the
> atomic state :-) .atomic_check() has to perform computations to verify
> the atomic commit, and storing the results in the commit's state
> prevents duplicating the same calculation at .atomic_commit() time.

Yes, you're right.

But it's still not good, as cdns_mhdp_validate_mode_params uses link details to do the calculations,
but we do link training only later and thus the calculations are invalid.

cdns_mhdp_validate_mode_params is also called from the HPD interrupt, and there it changes the
current bridge state. link_mutex is being held in every place where cdns_mhdp_validate_mode_params
is called, so I guess it's fine.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki