Re: [PATCH 11/11] drm/fbdevdrm: Detect and validate display modes

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Thomas Zimmermann <tzimmermann@suse.de>
Cc: airlied@linux.ie, linux-fbdev@vger.kernel.org,
	dri-devel@lists.freedesktop.org, b.zolnierkie@samsung.com
Subject: Re: [PATCH 11/11] drm/fbdevdrm: Detect and validate display modes
Date: Tue, 26 Mar 2019 16:47:41 +0000	[thread overview]
Message-ID: <20190326164741.GA3888@intel.com> (raw)
In-Reply-To: <20190326091744.11542-12-tzimmermann@suse.de>

On Tue, Mar 26, 2019 at 10:17:44AM +0100, Thomas Zimmermann wrote:
> Mode detection currently reports the modes listed in fb_info::modelist.
> The list is either build from EDID information or, more often, a list of
> previously set modes. A later update to the mode detection could also
> take into account the modes in fb_monspecs::modedb or test pre-defined
> VESA modes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/fbdevdrm/fbdevdrm_modeset.c | 163 +++++++++++++++++++-
>  1 file changed, 162 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/fbdevdrm/fbdevdrm_modeset.c b/drivers/gpu/drm/fbdevdrm/fbdevdrm_modeset.c
> index 87f56ec76edf..e89eca4b58df 100644
> --- a/drivers/gpu/drm/fbdevdrm/fbdevdrm_modeset.c
> +++ b/drivers/gpu/drm/fbdevdrm/fbdevdrm_modeset.c
> @@ -21,9 +21,16 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
>  #include <linux/fb.h>
> +#include "fbdevdrm_device.h"
>  #include "fbdevdrm_modes.h"
>  #include "fbdevdrm_primary.h"
>  
> +static struct fbdevdrm_modeset* fbdevdrm_modeset_of_connector(
> +	struct drm_connector *connector)
> +{
> +	return container_of(connector, struct fbdevdrm_modeset, connector);
> +}
> +
>  static struct fbdevdrm_modeset* fbdevdrm_modeset_of_crtc(
>  	struct drm_crtc *crtc)
>  {
> @@ -353,11 +360,130 @@ static const struct drm_encoder_funcs fbdevdrm_encoder_funcs = {
>   * Connector
>   */
>  
> -static int connector_helper_get_modes(struct drm_connector *connector)
> +static int update_display_info(struct drm_display_info *info, struct fb_info *fb_info)
> +{
> +	int num_pixel;
> +
> +	if (fb_info->fix.type != FB_TYPE_PACKED_PIXELS)
> +		return -EINVAL; /* rule out text mode, etc. */
> +
> +	if (fb_info->fix.id[0]) {
> +		strncpy(info->name, fb_info->fix.id, sizeof(info->name) - 1);
> +		info->name[sizeof(info->name) - 1] = '\0';
> +	} else {
> +		memset(info->name, '\0', sizeof(info->name));
> +	}
> +
> +	info->width_mm = fb_info->var.width;
> +	info->height_mm = fb_info->var.height;
> +	info->pixel_clock = PICOS2KHZ(fb_info->var.pixclock);
> +
> +	num_pixel = 0;
> +	if (fb_info->var.red.length)
> +		++num_pixel;
> +	if (fb_info->var.green.length)
> +		++num_pixel;
> +	if (fb_info->var.blue.length)
> +		++num_pixel;
> +	if (fb_info->var.transp.length)
> +		++num_pixel;
> +
> +	if (num_pixel)
> +		info->bpc = fb_info->var.bits_per_pixel;
> +	else
> +		info->bpc = 0;
> +
> +	info->subpixel_order = SubPixelUnknown;
> +	info->color_formats = DRM_COLOR_FORMAT_RGB444;
> +	info->bus_formats = &info->color_formats;
> +	info->num_bus_formats = 1;
> +	info->bus_flags = 0;
> +	info->max_tmds_clock = 0;
> +	info->dvi_dual = false;
> +	info->has_hdmi_infoframe = false;
> +	info->edid_hdmi_dc_modes = 0;
> +	info->cea_rev = 0;
> +	memset(&info->hdmi, 0, sizeof(info->hdmi));
> +	info->non_desktop = 0;

I think the only things here you may want to set are
width_mm and height_mm. The rest should not matter.

> +
> +	return 0;
> +}
> +
> +static int drm_mode_probed_add_from_fb_videomode(
> +	struct drm_connector *connector, const struct fb_videomode *fb_mode,
> +	struct fb_info *fb_info)
>  {
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_create_from_fb_videomode(connector->dev, fb_mode);
> +	if (!mode)
> +		return -ENOMEM;
> +
> +	mode->width_mm = fb_info->var.width;
> +	mode->height_mm = fb_info->var.height;
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	/* update connector properties from display mode */
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		connector->interlace_allowed = true;
> +	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
> +		connector->doublescan_allowed = true;
> +	if (mode->flags & DRM_MODE_FLAG_3D_MASK)
> +		connector->stereo_allowed = true;
> +
>  	return 0;
>  }
>  
> +static int connector_helper_get_modes(struct drm_connector *connector)
> +{
> +	struct fbdevdrm_modeset *modeset;
> +	struct list_head *pos;
> +	int ret, num_modes = 0;
> +
> +	modeset = fbdevdrm_modeset_of_connector(connector);
> +
> +	ret = update_display_info(&connector->display_info, modeset->fb_info);
> +	if (ret)
> +		return 0;
> +
> +	/* update connector properties from video modes */
> +	connector->interlace_allowed = 0;
> +	connector->doublescan_allowed = 0;
> +	connector->stereo_allowed = 0;
> +
> +	if (!num_modes && modeset->fb_info->mode) {
> +		ret = drm_mode_probed_add_from_fb_videomode(
> +			connector, modeset->fb_info->mode, modeset->fb_info);
> +		if (!ret)
> +			++num_modes;
> +	}
> +
> +	if (!num_modes) {
> +
> +		/* DRM backporting notes: we go through all modes in the
> +		 * fb_info's mode list and convert each to a DRM modes. If
> +		 * you convert an fbdev driver to DRM, replace this code
> +		 * with an actual hardware query. This will usually involve
> +		 * reading the monitor EDID via DDC.
> +		 */
> +
> +		list_for_each(pos, &modeset->fb_info->modelist) {

fbdev has a modelist? I guess it does. But not exposed to
userspace, which is probably the reason I never realized this.

-- 
Ville Syrjälä
Intel