Re: [PATCH v2 12/13] drm/bridge: lt9611: stop filtering modes via the table

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 12/01/2023 10:43, neil.armstrong@xxxxxxxxxx wrote:
> On 11/01/2023 16:37, Dmitry Baryshkov wrote:
> > On 11/01/2023 12:57, Neil Armstrong wrote:
> > > On 08/01/2023 17:56, Dmitry Baryshkov wrote:
> > > > The lt9611 bridge can support different modes, it makes no sense to 
> > > > list
> > > > them in the table. Drop the table and check the number of interfaces
> > > > using the fixed value.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > ---
> > > >   drivers/gpu/drm/bridge/lontium-lt9611.c | 41 
> > > > +++----------------------
> > > >   1 file changed, 4 insertions(+), 37 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c 
> > > > b/drivers/gpu/drm/bridge/lontium-lt9611.c
> > > > index 82af1f954cc6..df9f015aa3a0 100644
> > > > --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> > > > +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> > > > @@ -84,24 +84,6 @@ static const struct regmap_config 
> > > > lt9611_regmap_config = {
> > > >       .num_ranges = ARRAY_SIZE(lt9611_ranges),
> > > >   };
> > > > -struct lt9611_mode {
> > > > -    u16 hdisplay;
> > > > -    u16 vdisplay;
> > > > -    u8 vrefresh;
> > > > -    u8 lanes;
> > > > -    u8 intfs;
> > > > -};
> > > > -
> > > > -static struct lt9611_mode lt9611_modes[] = {
> > > > -    { 3840, 2160, 30, 4, 2 }, /* 3840x2160 24bit 30Hz 4Lane 2ports */
> > > > -    { 1920, 1080, 60, 4, 1 }, /* 1080P 24bit 60Hz 4lane 1port */
> > > > -    { 1920, 1080, 30, 3, 1 }, /* 1080P 24bit 30Hz 3lane 1port */
> > > > -    { 1920, 1080, 24, 3, 1 },
> > > > -    { 720, 480, 60, 4, 1 },
> > > > -    { 720, 576, 50, 2, 1 },
> > > > -    { 640, 480, 60, 2, 1 },
> > > > -};
> > > > -
> > > >   static struct lt9611 *bridge_to_lt9611(struct drm_bridge *bridge)
> > > >   {
> > > >       return container_of(bridge, struct lt9611, bridge);
> > > > @@ -603,21 +585,6 @@ static int lt9611_regulator_enable(struct 
> > > > lt9611 *lt9611)
> > > >       return 0;
> > > >   }
> > > > -static struct lt9611_mode *lt9611_find_mode(const struct 
> > > > drm_display_mode *mode)
> > > > -{
> > > > -    int i;
> > > > -
> > > > -    for (i = 0; i < ARRAY_SIZE(lt9611_modes); i++) {
> > > > -        if (lt9611_modes[i].hdisplay == mode->hdisplay &&
> > > > -            lt9611_modes[i].vdisplay == mode->vdisplay &&
> > > > -            lt9611_modes[i].vrefresh == drm_mode_vrefresh(mode)) {
> > > > -            return &lt9611_modes[i];
> > > > -        }
> > > > -    }
> > > > -
> > > > -    return NULL;
> > > > -}
> > > > -
> > > >   static enum drm_connector_status lt9611_bridge_detect(struct 
> > > > drm_bridge *bridge)
> > > >   {
> > > >       struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> > > > @@ -832,12 +799,12 @@ static enum drm_mode_status 
> > > > lt9611_bridge_mode_valid(struct drm_bridge *bridge,
> > > >                                const struct drm_display_info *info,
> > > >                                const struct drm_display_mode *mode)
> > > >   {
> > > > -    struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode);
> > > >       struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
> > > > -    if (!lt9611_mode)
> > > > -        return MODE_BAD;
> > > > -    else if (lt9611_mode->intfs > 1 && !lt9611->dsi1)
> > > > +    if (mode->hdisplay >= 3840 && drm_mode_vrefresh(mode) >= 31)
> > >
> > > Isn't 31 a typo ?
> >
> > Maybe I should change that to drm_mode_vrefresh(mode) > 30. The chip 
> > supports 3840x2160-30, but doesn't promise to support anything above 
> > that.
>
> Yep >= 31 is valid, but > 30 seems more logical.
>
> Concerning the hdisplay check, shouldn't be separate ?
>
> You should switch to:
> if (mode->hdisplay > 3840)
>     return MODE_BAD_WIDTH;
>
> if (mode->hdisplay == 3840 && drm_mode_vrefresh(mode) > 30)
>     return MODE_CLOCK_HIGH;

Good idea, I'll adapt it for v3.

> Isn't there limits on vdisplay aswell ?

I don't see any special limit in the datasheet. HDMI 1.4, 4k@30, that's 
all. I think I'll just add vdisply > 2160 check next to hdisplay.

> Neil
>
> > > > +        return MODE_CLOCK_HIGH;
> > > > +
> > > > +    if (mode->hdisplay > 2000 && !lt9611->dsi1_node)
> > > >           return MODE_PANEL;
> > > >       else
> > > >           return MODE_OK;

--
With best wishes
Dmitry