Hi, On Tue, Mar 14, 2023 at 4:55 PM Jianhua Lu <lujianhua000@xxxxxxxxx> wrote: > > On Tue, Mar 14, 2023 at 10:12:02AM -0700, Doug Anderson wrote: > > Hi, > > > > On Tue, Mar 14, 2023 at 4:45 AM Jianhua Lu <lujianhua000@xxxxxxxxx> 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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx/ > > 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 == 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 == 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
