lkml.org 
[lkml]   [2023]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation
    Hi,

    On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
    > As per TRM this controller supports pixelclocks starting from 25 MHz. The
    > maximum supported pixelclocks are defined by the phy configurations we
    > have. Also it can't support modes that require doubled clocks.
    > If there is a phy reference clock we can additionally validate against
    > VESA DMT's recommendations.
    > Those checks are added to the mode_valid hook of the connector and
    > encoder's mode_fixup hook.
    >
    > Signed-off-by: Alex Bee <knaerzche@gmail.com>
    > ---
    > drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
    > 1 file changed, 36 insertions(+), 2 deletions(-)
    >
    > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
    > index f7f0bec725f9..2f839ff31c1c 100644
    > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
    > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
    > @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
    > struct inno_hdmi_phy_config *default_phy_config;
    > };
    >
    > +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
    > +
    > struct hdmi_data_info {
    > int vic;
    > bool sink_has_audio;
    > @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
    > return 0;
    > }
    >
    > +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
    > + struct drm_display_mode *mode)
    > +{

    So, mode_valid is only called to filter out the modes retrieved by
    get_modes, but it won't be called when userspace programs a mode. That's
    atomic_check's job.

    So you probably want to create a shared function between atomic_check
    and mode_valid, and call it from both places (or call mode_valid from
    atomic_check).

    > + /* No support for double-clock modes */
    > + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
    > + return MODE_BAD;
    > +
    > + unsigned int mpixelclk = mode->clock * 1000;

    Variables should be declared at the top of the function.

    > + if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
    > + return MODE_CLOCK_LOW;

    You probably want to check the max TMDS clock too?

    > + if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
    > + return MODE_CLOCK_HIGH;
    > +
    > + if (hdmi->refclk) {
    > + long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
    > + unsigned int max_tolerance = mpixelclk / 5000;
    > +
    > + /* Vesa DMT standard mentions +/- 0.5% max tolerance */
    > + if (abs(refclk - mpixelclk) > max_tolerance ||
    > + mpixelclk - refclk > max_tolerance;
    > + return MODE_NOCLOCK;

    You should use abs_diff here. abs() will get confused by the unsigned vs
    signed comparison.

    > + }
    > +
    > + return MODE_OK;
    > +}
    > +
    > static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
    > struct drm_display_mode *mode,
    > struct drm_display_mode *adj_mode)
    > @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
    > const struct drm_display_mode *mode,
    > struct drm_display_mode *adj_mode)
    > {
    > - return true;
    > + struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
    > +
    > + return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
    > }

    Why do you call mode_valid in mode_fixup?

    Maxime
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2023-12-14 09:07    [W:4.179 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site