Hi,

Thanks for working on that, it's much better now.

On Sat, Dec 16, 2023 at 05:26:29PM +0100, Alex Bee wrote:
> +static struct drm_connector_state *
> +inno_hdmi_connector_duplicate_state(struct drm_connector *connector)
> +{
> + struct inno_hdmi_connector_state *inno_conn_state;
> +
> + if (WARN_ON(!connector->state))
> + return NULL;
> +
> + inno_conn_state = kmemdup(to_inno_hdmi_conn_state(connector->state),
> + sizeof(*inno_conn_state), GFP_KERNEL);
> +
> + if (!inno_conn_state)
> + return NULL;
> +
> + __drm_atomic_helper_connector_duplicate_state(connector,
> + &inno_conn_state->base);
> +
> + return &inno_conn_state->base;
> +}
> +
> static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
> .fill_modes = inno_hdmi_probe_single_connector_modes,
> .detect = inno_hdmi_connector_detect,
> .destroy = inno_hdmi_connector_destroy,
> .reset = drm_atomic_helper_connector_reset,
> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_duplicate_state = inno_hdmi_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };

You also need a custom reset and atomic_destroy_state implementations

Maxime

Attachments:

(No filename) (1.24 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-18 09:05:18

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 <[email protected]>
> ---
> 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.

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.

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

Attachments:

(No filename) (4.29 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-18 12:33:09

by Alex Bee

[permalink] [raw]

Subject: Re: [PATCH v2 18/27] drm/rockchip: inno_hdmi: Subclass connector state

Hi,

Am 18.12.23 um 10:02 schrieb Maxime Ripard:
> Hi,
>
> Thanks for working on that, it's much better now.
>
> On Sat, Dec 16, 2023 at 05:26:29PM +0100, Alex Bee wrote:
>> +static struct drm_connector_state *
>> +inno_hdmi_connector_duplicate_state(struct drm_connector *connector)
>> +{
>> + struct inno_hdmi_connector_state *inno_conn_state;
>> +
>> + if (WARN_ON(!connector->state))
>> + return NULL;
>> +
>> + inno_conn_state = kmemdup(to_inno_hdmi_conn_state(connector->state),
>> + sizeof(*inno_conn_state), GFP_KERNEL);
>> +
>> + if (!inno_conn_state)
>> + return NULL;
>> +
>> + __drm_atomic_helper_connector_duplicate_state(connector,
>> + &inno_conn_state->base);
>> +
>> + return &inno_conn_state->base;
>> +}
>> +
>> static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
>> .fill_modes = inno_hdmi_probe_single_connector_modes,
>> .detect = inno_hdmi_connector_detect,
>> .destroy = inno_hdmi_connector_destroy,
>> .reset = drm_atomic_helper_connector_reset,
>> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> + .atomic_duplicate_state = inno_hdmi_connector_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> };
> You also need a custom reset and atomic_destroy_state implementations
Yea sure - thanks for the heads up.
I had this in multiple commits locally and thought I squashed them into
this one. Well, I obviously didn't :)

Alex
> Maxime

2023-12-18 12:37:59

by Alex Bee

[permalink] [raw]

Subject: Re: [PATCH v2 20/27] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Am 18.12.23 um 10:05 schrieb Maxime Ripard:
> On Sat, Dec 16, 2023 at 05:26:31PM +0100, Alex Bee wrote:
>> @@ -431,6 +452,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>> else
>> inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
>>
>> + inno_conn_state->rgb_limited_range =
>> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>> +
>> /* Mute video and audio output */
>> hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
>> v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
> This needs to be done at atomic_check time: the expectation is that by
> the time you commit the state, everything is prepared for it.
OK. I guess that also applies to the other members of
inno_hdmi_connector_state (former hdmi_data) and was wrong all the time.

Alex
> Maxime

2023-12-18 13:14:11

by Alex Bee

[permalink] [raw]

Subject: Re: [PATCH v2 19/27] drm/rockchip: inno_hdmi: Move tmds rate to connector state subclass

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 <[email protected]>
>> ---
>> 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.
>
> 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

2023-12-18 13:22:27

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2 20/27] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

On Mon, Dec 18, 2023 at 01:37:47PM +0100, Alex Bee wrote:
>
> Am 18.12.23 um 10:05 schrieb Maxime Ripard:
> > On Sat, Dec 16, 2023 at 05:26:31PM +0100, Alex Bee wrote:
> > > @@ -431,6 +452,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > else
> > > inno_conn_state->colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > + inno_conn_state->rgb_limited_range =
> > > + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > +
> > > /* Mute video and audio output */
> > > hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
> > > v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
> > This needs to be done at atomic_check time: the expectation is that by
> > the time you commit the state, everything is prepared for it.
> OK. I guess that also applies to the other members of
> inno_hdmi_connector_state (former hdmi_data) and was wrong all the time.

Yeah, this will apply to all the members of inno_hdmi_connector_state indeed

Maxime

Attachments:

(No filename) (998.00 B)
signature.asc (235.00 B)
Download all attachments

2023-12-22 17:52:39

by Alex Bee

[permalink] [raw]

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 <[email protected]>
>>> ---
>>> 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