Message-ID: <ebe86636-95aa-4766-b8d8-de5d81218537@gmail.com>
Date: Fri, 22 Dec 2023 18:51:49 +0100
From: Alex Bee <knaerzche@...il.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Sandy Huang <hjc@...k-chips.com>, Heiko Stübner
 <heiko@...ech.de>, Andy Yan <andyshrk@....com>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Daniel Vetter <daniel@...ll.ch>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 19/27] drm/rockchip: inno_hdmi: Move tmds rate to
 connector state subclass

Hi Maxime,

Am 18.12.23 um 13:49 schrieb Alex Bee:
>
> Am 18.12.23 um 11:06 schrieb Maxime Ripard:
>> Hi,
>>
>> On Sat, Dec 16, 2023 at 05:26:30PM +0100, Alex Bee wrote:
>>> Similar to the othter members of inno_hdmi_connector_state the 
>>> tmds_rate is
>>> not a property of the device, but of the connector state. Move it to
>>> inno_hdmi_connector_state and make it a long to comply with the clock
>>> framework. To get arround the issue of not having the connector 
>>> state when
>>> inno_hdmi_i2c_init is called in the bind path, getting the tmds rate is
>>> wrapped in function which returns the fallback rate if the connector
>>> doesn't have a state yet.
>>>
>>> Signed-off-by: Alex Bee <knaerzche@...il.com>
>>> ---
>>> changes in v2:
>>>   - new patch
>>>
>>>   drivers/gpu/drm/rockchip/inno_hdmi.c | 36 
>>> +++++++++++++++++++---------
>>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c 
>>> b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>> index f9bfae1e97a2..6799d24501b8 100644
>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>> @@ -47,14 +47,13 @@ struct inno_hdmi {
>>>         struct inno_hdmi_i2c *i2c;
>>>       struct i2c_adapter *ddc;
>>> -
>>> -    unsigned int tmds_rate;
>>>   };
>>>     struct inno_hdmi_connector_state {
>>>       struct drm_connector_state    base;
>>>       unsigned int            enc_out_format;
>>>       unsigned int            colorimetry;
>>> +    unsigned long            tmds_rate;
>>>   };
>>>     static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder 
>>> *encoder)
>>> @@ -133,11 +132,33 @@ static inline void hdmi_modb(struct inno_hdmi 
>>> *hdmi, u16 offset,
>>>       hdmi_writeb(hdmi, offset, temp);
>>>   }
>>>   +static unsigned long inno_hdmi_tmds_rate(struct inno_hdmi *hdmi)
>>> +{
>>> +    struct drm_connector *connector = &hdmi->connector;
>>> +    struct drm_connector_state *conn_state = connector->state;
>>> +    struct inno_hdmi_connector_state *inno_conn_state;
>>> +
>>> +    if (conn_state) {
>>> +        inno_conn_state = to_inno_hdmi_conn_state(conn_state);
>>> +        return inno_conn_state->tmds_rate;
>>> +    }
>>> +
>>> +    /*
>>> +     * When IP controller haven't configured to an accurate video
>>> +     * timing, then the TMDS clock source would be switched to
>>> +     * PCLK_HDMI, so we need to init the TMDS rate to PCLK rate,
>>> +     * and reconfigure the DDC clock.
>>> +     */
>>> +
>>> +    return clk_get_rate(hdmi->pclk);
>>> +}
>>> +
>>>   static void inno_hdmi_i2c_init(struct inno_hdmi *hdmi)
>>>   {
>>>       int ddc_bus_freq;
>>> +    unsigned long tmds_rate = inno_hdmi_tmds_rate(hdmi);
>>>   -    ddc_bus_freq = (hdmi->tmds_rate >> 2) / HDMI_SCL_RATE;
>>> +    ddc_bus_freq = (tmds_rate >> 2) / HDMI_SCL_RATE;
>>>         hdmi_writeb(hdmi, DDC_BUS_FREQ_L, ddc_bus_freq & 0xFF);
>>>       hdmi_writeb(hdmi, DDC_BUS_FREQ_H, (ddc_bus_freq >> 8) & 0xFF);
>>> @@ -431,7 +452,7 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>        * DCLK_LCDC, so we need to init the TMDS rate to mode pixel
>>>        * clock rate, and reconfigure the DDC clock.
>>>        */
>>> -    hdmi->tmds_rate = mode->clock * 1000;
>>> +    inno_conn_state->tmds_rate = mode->clock * 1000;
>>>       inno_hdmi_i2c_init(hdmi);
>>>         /* Unmute video and audio output */
>>> @@ -823,13 +844,6 @@ static int inno_hdmi_bind(struct device *dev, 
>>> struct device *master,
>>>           goto err_disable_clk;
>>>       }
>>>   -    /*
>>> -     * When IP controller haven't configured to an accurate video
>>> -     * timing, then the TMDS clock source would be switched to
>>> -     * PCLK_HDMI, so we need to init the TMDS rate to PCLK rate,
>>> -     * and reconfigure the DDC clock.
>>> -     */
>>> -    hdmi->tmds_rate = clk_get_rate(hdmi->pclk);
>>>       inno_hdmi_i2c_init(hdmi);
>> I still think my patch is better there.
>>
I found a way now and added your patch which removes the tmds_rate: I
noticed powering up the phy initially isn't necessary at all (and makes no
sense). I refactored inno_hdmi_set_pwr_mode in two functions (in a similar
way you did for the infoframe), so that the tmds rate (pixelclock) is only
needed as parameter when a mode is set.

Alex

>> There's two places that use the inno_hdmi.tmds_rate field: the two
>> callers of inno_hdmi_i2c_init(). One is at bind time and needs to
>> initialise it with a sane default since we don't have a mode set yet,
>> the other is to update the internal clock rate while we have a mode set.
> That’s, unfortunately, not fully true: inno_hdmi_set_pwr_mode not only
> called at mode_set-time, but also in inno_hdmi_reset which is called 
> in the
> bind path (where we do not have a mode). That’s the point why I thought
> extracting this in function makes sense. Otherwise I would have to 
> pass the
> tmds_rate to inno_hdmi_set_pwr_mode (also for the LOWER_PWR-case where I
> don't need it) or do that whole fallback-if-no-mode thing in
> inno_hdmi_set_pwr_mode directly. Neither would make the code easier to
> follow. Being able to use it in inno_hdmi_i2c_init also is a nice 
> gimmick.
> I agree, having it in the custom connector state is not strictly 
> required,
> but I'd really like to keep the wrapping function.
>
> Alex
>
>> Since there's a single "modeset" user, there's no need to store it in
>> the state structure at all: it can be a local variable.
>>
>> And in the bind function, you're not going to use the state structure
>> either since there's no state, and it's just a default that has no
>> relation to the modeset code at all.
>>
>> Your function on the other end tries to reconcile and handle the two.
>> But there's no reason to, it just makes the code harder to follow. Just
>> pass the parent rate you want to init with as an argument and it's easy
>> to read and maintain.
>>
>> Maxime

Powered by blists - more mailing lists