From: Thierry Reding <thierry.reding@gmail.com>
To: Stefan Mavrodiev <stefan.mavrodiev@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
Stefan Mavrodiev <stefan@olimex.com>,
David Airlie <airlied@linux.ie>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Randy Dunlap <rdunlap@infradead.org>,
open list <linux-kernel@vger.kernel.org>,
"open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
Rob Herring <robh+dt@kernel.org>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v2 1/1] drm/panel: Add support for Olimex LCD-OLinuXino panel
Date: Tue, 10 Jul 2018 16:48:45 +0200 [thread overview]
Message-ID: <20180710144845.GA25784@ulmo> (raw)
In-Reply-To: <e1229a90-1bdb-0c8e-c371-cde0bc30ea56@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 4344 bytes --]
On Tue, Jul 10, 2018 at 04:08:54PM +0300, Stefan Mavrodiev wrote:
> On 07/10/2018 01:32 PM, Thierry Reding wrote:
> > On Mon, Jun 25, 2018 at 09:44:35AM +0300, Stefan Mavrodiev wrote:
[...]
> > > diff --git a/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c b/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c
[...]
> > > +static int lcd_olinuxino_get_modes(struct drm_panel *panel)
> > > +{
> > > + struct lcd_olinuxino *lcd = to_lcd_olinuxino(panel);
> > > + struct drm_connector *connector = lcd->panel.connector;
> > > + struct lcd_olinuxino_info *lcd_info = &lcd->eeprom.info;
> > > + struct drm_device *drm = lcd->panel.drm;
> > > + struct lcd_olinuxino_mode *lcd_mode;
> > > + struct drm_display_mode *mode;
> > > + int i, num = 0;
> > These two can be unsigned.
> >
> > > +
> > > + /* Read up to 4 modes */
> > > + for (i = 0; i < lcd->eeprom.num_modes && i < 4; i++) {
> > Can it happen that lcd->eeprom.num_modes >= 4? Seems to me like that
> > would be a serious bug. Perhaps move that check to where the EEPROM is
> > read and output a warning, then overwrite lcd->eeprom.num_modes with 4?
> If num_modes is bigger than 4, then lcd_mode pointer will point to invalid
> location. This can happen if someone overwrite the eeprom and apply
> correct checksum at the end. Then i < 4 makes sure this won't happen.
I still think that this should be checked earlier in the code, like at
->probe() time. That way you can output a warning once so that people
have a chance to notice a broken EEPROM and potentially do something
about it. You can then also sanitize the EEPROM contents so that the
rest of the code (i.e. ->get_modes()) doesn't have to check for this
error case.
> >
> > > + lcd_mode = (struct lcd_olinuxino_mode *)
> > > + &lcd->eeprom.reserved[i * sizeof(*lcd_mode)];
> > > +
> > > + mode = drm_mode_create(drm);
> > > + if (!mode) {
> > > + dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
> > > + lcd_mode->hactive,
> > > + lcd_mode->vactive,
> > > + lcd_mode->refresh);
> > > + continue;
> > > + }
> > > +
> > > + mode->clock = lcd_mode->pixelclock;
> > > + mode->hdisplay = lcd_mode->hactive;
> > > + mode->hsync_start = lcd_mode->hactive + lcd_mode->hfp;
> > > + mode->hsync_end = lcd_mode->hactive + lcd_mode->hfp +
> > > + lcd_mode->hpw;
> > > + mode->htotal = lcd_mode->hactive + lcd_mode->hfp +
> > > + lcd_mode->hpw + lcd_mode->hbp;
> > > + mode->vdisplay = lcd_mode->vactive;
> > > + mode->vsync_start = lcd_mode->vactive + lcd_mode->vfp;
> > > + mode->vsync_end = lcd_mode->vactive + lcd_mode->vfp +
> > > + lcd_mode->vpw;
> > > + mode->vtotal = lcd_mode->vactive + lcd_mode->vfp +
> > > + lcd_mode->vpw + lcd_mode->vbp;
> > > + mode->vrefresh = lcd_mode->refresh;
> > > +
> > > + if (lcd->eeprom.num_modes == 1)
> > > + mode->type |= DRM_MODE_TYPE_PREFERRED;
> > Is there no way to determine the preferred mode if there are more than
> > one? Perhaps always prefer the first mode?
> My idea here is what if the first mode is not supported by the host? That's
> why
> we will be storing multiple modes, one with the most strict timings
> (e.g rounded to 10kHz or less, according to the lcd datesheet), and others
> with less strict.
Its not the panel driver's responsibility to take into account the host
capabilities. The panel driver should, to the best of its abilities,
report the supported modes and the host driver is responsible for
dealing with the mode list. If any of the modes are not within its
capabilities, then it can filter them out (using the ->mode_valid()
callback).
In this particular case, if there is no clearly defined preferred mode
I'd argue that either you don't mark any mode as preferred, or you
choose the one with the strictest timings. I had hoped that perhaps the
first mode would always be the one with the strictest timings and hence
would be the preferred mode, but it seems like that's not a guarantee.
If so, it's pretty much impossible for the driver to determine a
preferred mode, so can't do much about it.
As for the special case of a single mode being present in the EEPROM,
I'm not sure if that's something worth considering. If there's only one
it doesn't really matter that it's preferred.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
prev parent reply other threads:[~2018-07-10 14:48 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-25 6:44 [PATCH v2 1/1] drm/panel: Add support for Olimex LCD-OLinuXino panel Stefan Mavrodiev
2018-06-25 20:41 ` Rob Herring
2018-07-10 10:32 ` Thierry Reding
2018-07-10 13:08 ` Stefan Mavrodiev
2018-07-10 13:27 ` Greg Kroah-Hartman
2018-07-10 13:41 ` Stefan Mavrodiev
2018-07-10 14:48 ` Thierry Reding [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180710144845.GA25784@ulmo \
--to=thierry.reding@gmail.com \
--cc=airlied@linux.ie \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mchehab+samsung@kernel.org \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
--cc=stefan.mavrodiev@gmail.com \
--cc=stefan@olimex.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).