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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 4 Sep 2020 08:53:40 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC:     Swapnil Jakhade <sjakhade@...ence.com>,
        <dri-devel@...ts.freedesktop.org>, <airlied@...ux.ie>,
        <daniel@...ll.ch>, <robh+dt@...nel.org>, <a.hajda@...sung.com>,
        <narmstrong@...libre.com>, <jonas@...boo.se>,
        <jernej.skrabec@...l.net>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <mparab@...ence.com>,
        <yamonkar@...ence.com>, <jsarha@...com>, <nsekhar@...com>,
        <praneeth@...com>, <nikhil.nd@...com>
Subject: Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546
 DPI/DP bridge

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