[PATCH v2 1/1] drm/panel: Add support for Olimex LCD-OLinuXino panel

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180710/f08053dc/attachment.sig>