MIME-Version: 1.0
References: <20230314114451.8872-1-lujianhua000@gmail.com> <CAD=FV=X7K2KQoRCsr17kj-DWiPs7h_zfcYxK_cdBVnC0wR1NwA@mail.gmail.com>
 <ZBEJdX6WVZ3Kqdo0@Gentoo>
In-Reply-To: <ZBEJdX6WVZ3Kqdo0@Gentoo>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 16 Mar 2023 17:23:46 -0700
Message-ID: <CAD=FV=VgWSvrpCdi2pRr0JctHxcq1-qNJSMAxEVva6nEnv3e_g@mail.gmail.com>
Subject: Re: [PATCH] drm/panel: Fix panel mode type setting logic
To:     Jianhua Lu <lujianhua000@gmail.com>
Cc:     Neil Armstrong <neil.armstrong@linaro.org>,
        Sam Ravnborg <sam@ravnborg.org>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,


On Tue, Mar 14, 2023 at 4:55=E2=80=AFPM Jianhua Lu <lujianhua000@gmail.com>=
 wrote:
>
> On Tue, Mar 14, 2023 at 10:12:02AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Mar 14, 2023 at 4:45=E2=80=AFAM Jianhua Lu <lujianhua000@gmail.=
com> wrote:
> > >
> > > Some panels set mode type to DRM_MODE_TYPE_PREFERRED by the number
> > > of modes. It isn't reasonable, so set the first mode type to
> > > DRM_MODE_TYPE_PREFERRED. This should be more reasonable.
> > >
> > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com>
> > > ---
> > >  drivers/gpu/drm/panel/panel-abt-y030xx067a.c     | 2 +-
> > >  drivers/gpu/drm/panel/panel-auo-a030jtn01.c      | 2 +-
> > >  drivers/gpu/drm/panel/panel-edp.c                | 4 ++--
> > >  drivers/gpu/drm/panel/panel-innolux-ej030na.c    | 2 +-
> > >  drivers/gpu/drm/panel/panel-newvision-nv3051d.c  | 2 +-
> > >  drivers/gpu/drm/panel/panel-newvision-nv3052c.c  | 2 +-
> > >  drivers/gpu/drm/panel/panel-novatek-nt35950.c    | 2 +-
> > >  drivers/gpu/drm/panel/panel-novatek-nt39016.c    | 2 +-
> > >  drivers/gpu/drm/panel/panel-orisetech-ota5601a.c | 2 +-
> > >  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c      | 4 ++--
> > >  drivers/gpu/drm/panel/panel-simple.c             | 4 ++--
> > >  11 files changed, 14 insertions(+), 14 deletions(-)
> >
> > Can you explain more about your motivation here? At least for
> This demonstrates a bad way to set DRM_MODE_TYPE_PREFERRED for panels
> with more than one mode. It mislead the future contributors to send
> a patch with this piece of code. There is also a discussion for it.
> https://lore.kernel.org/lkml/904bc493-7160-32fd-9709-1dcb978ddbab@linaro.=
org/
> > panel-edp and panel-simple it seems like it would be better to leave
> > the logic alone and manually add DRM_MODE_TYPE_PREFERRED to the right
> > mode for the rare panel that actually has more than one mode listed.
> I think we can order it to the first mode if the mode type should be
> DRM_MODE_TYPE_PREFERRED, It's also same.

A pointer to the original discussion would have been super helpful to
be provided in your patch description.

Personally, I still stand by my assertion that I'd rather that
DRM_MODE_TYPE_PREFERRED be placed in the actual data instead of being
done like this in post-processing. To me the old check for "num_modes
=3D=3D 1" is basically saying that the people creating the "static const"
data in this file were lazy and didn't feel like they needed to set a
DRM_MODE_TYPE_PREFERRED when there was only one mode listed. Thus, we
can add it for them. When "num_modes" is more than 1 then we shouldn't
allow the people making the "static const" data to be lazy. We should
force them to set one of the modes to be PREFERRED rather than for
them to have to know about this magic rule.

Thus, for me, my order of preference would be these (note, I've mostly
looked at panel-edp and panel-simple):

1. Leave the check as "num_modes =3D=3D 1" and document that we're
basically allowing the people writing the "static const" structure to
be lazy if there's only one mode. Manually add the
DRM_MODE_TYPE_PREFERRED flag to the small number of cases where there
is more than one mode. Possibly add a warning printout if we end up
without a PREFERRED mode. I'd give a Reviewed-by for this.

2. Fully get rid of this logic and add DRM_MODE_TYPE_PREFERRED to all
of the "static const" data. I guess maybe we can't do that for the
"timings" in panel-edp and panel-simple. I guess for those I'd be OK
with just setting PREFERRED on the first timing like your patch is
doing. I'd give a Reviewed-by for this.

3. Your patch. I won't NAK it since it seems like this is what other
(more senior) DRM folks were suggesting. ...but I don't love it. I
won't give a Reviewed-by for this but won't object to someone else
doing so.

-Doug