linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Ivan Mironov <mironov.ivan@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <maxime.ripard@bootlin.com>,
	Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
	saahriktu <mail@saahriktu.org>,
	Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
Subject: Re: [PATCH v1 2/2] drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock
Date: Mon, 7 Jan 2019 11:10:22 +0100	[thread overview]
Message-ID: <20190107101022.GA21184@phenom.ffwll.local> (raw)
In-Reply-To: <b9f21efe2fe2d0a71ad6672fbe9155df52ac5e02.camel@gmail.com>

On Sat, Jan 05, 2019 at 09:21:21PM +0500, Ivan Mironov wrote:
> On Fri, 2018-12-28 at 13:06 +0100, Daniel Vetter wrote:
> > On Fri, Dec 28, 2018 at 04:13:08AM +0500, Ivan Mironov wrote:
> > > Strict requirement of pixclock to be zero breaks support of SDL 1.2
> > > which contains hardcoded table of supported video modes with non-zero
> > > pixclock values[1].
> > > 
> > > To better understand which pixclock values are considered valid and how
> > > driver should handle these values, I briefly examined few existing fbdev
> > > drivers and documentation in Documentation/fb/. And it looks like there
> > > are no strict rules on that and actual behaviour varies:
> > > 
> > > 	* some drivers treat (pixclock == 0) as "use defaults" (uvesafb.c);
> > > 	* some treat (pixclock == 0) as invalid value which leads to
> > > 	  -EINVAL (clps711x-fb.c);
> > > 	* some pass converted pixclock value to hardware (uvesafb.c);
> > > 	* some are trying to find nearest value from predefined table
> > >           (vga16fb.c, video_gx.c).
> > > 
> > > Given this, I believe that it should be safe to just ignore this value if
> > > changing is not supported. It seems that any portable fbdev application
> > > which was not written only for one specific device working under one
> > > specific kernel version should not rely on any particular behaviour of
> > > pixclock anyway.
> > > 
> > > However, while enabling SDL1 applications to work out of the box when
> > > there is no /etc/fb.modes with valid settings, this change affects the
> > > video mode choosing logic in SDL. Depending on current screen
> > > resolution, contents of /etc/fb.modes and resolution requested by
> > > application, this may lead to user-visible difference (not always):
> > > image will be displayed in a right way, but it will be aligned to the
> > > left instead of center. There is no "right behaviour" here as well, as
> > > emulated fbdev, opposing to old fbdev drivers, simply ignores any
> > > requsts of video mode changes with resolutions smaller than current.
> > > 
> > > Feel free to NAK this patch if you think that it causes breakage of
> > > user-space =).
> > 
> > It's a regression, we don't nack regression fixes :-)
> > 
> > > The easiest way to reproduce this problem is to install sdl-sopwith[2],
> > > remove /etc/fb.modes file if it exists, and then try to run sopwith
> > > from console without X. At least in Fedora 29, sopwith may be simply
> > > installed from standard repositories.
> > > 
> > > [1] SDL 1.2.15 source code, src/video/fbcon/SDL_fbvideo.c, vesa_timings
> > > [2] http://sdl-sopwith.sourceforge.net/
> > > 
> > > Signed-off-by: Ivan Mironov <mironov.ivan@gmail.com>
> > 
> > I thought this is also a regression fix, so also needs Fixes: and cc:
> > stable?
> > -Daniel
> 
> This particular patch fixes a bug that existed for 10 years unnoticed.
> Are "Fixes:" and "Cc: stable" really required?
> 
> "pixclock != 0" check was introduced in this file by 785b93ef8c309
> ("drm/kms: move driver specific fb common code to helper functions
> (v2)"). But actually similar code existed in the device-specific
> drivers even earlier.

Afaui 785b93ef8c309 broke existing userspace. Broken userspace, but
existing userspace, and hence we need to fix this regression. That's the
rules with linux upstream, existing userspace is never allowed to break,
even if it does really stupid things. Hence Cc: stable.
-Daniel

> 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_fb_helper.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index aff576c3c4fb..b95a0c23c7c8 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -1690,9 +1690,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > >  	struct drm_fb_helper *fb_helper = info->par;
> > >  	struct drm_framebuffer *fb = fb_helper->fb;
> > >  
> > > -	if (var->pixclock != 0 || in_dbg_master())
> > > +	if (in_dbg_master())
> > >  		return -EINVAL;
> > >  
> > > +	if (var->pixclock != 0) {
> > > +		DRM_DEBUG("fbdev emulation doesn't support changing the pixel clock, value of pixclock is ignored\n");
> > > +		var->pixclock = 0;
> > > +	}
> > > +
> > >  	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > >  	    (drm_format_info_block_height(fb->format, 0) > 1))
> > >  		return -EINVAL;
> > > -- 
> > > 2.20.1
> > > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

  reply	other threads:[~2019-01-07 10:11 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-27 23:13 [PATCH v1 0/2] Fix SDL 1.2 on emulated fbdev devices (broken in kernels >=4.19) Ivan Mironov
2018-12-27 23:13 ` [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of SDL 1.2 Ivan Mironov
2018-12-28 12:15   ` Daniel Vetter
2019-01-05 16:11     ` Ivan Mironov
2019-01-07 10:08       ` Daniel Vetter
2019-01-08  7:26         ` Ivan Mironov
2018-12-28 17:28   ` Eugeniy Paltsev
2018-12-27 23:13 ` [PATCH v1 2/2] drm/fb-helper: Ignore the value of fb_var_screeninfo.pixclock Ivan Mironov
2018-12-28 12:06   ` Daniel Vetter
2019-01-05 16:21     ` Ivan Mironov
2019-01-07 10:10       ` Daniel Vetter [this message]
2019-01-07 10:12       ` Greg KH

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=20190107101022.GA21184@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=Eugeniy.Paltsev@synopsys.com \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mail@saahriktu.org \
    --cc=maxime.ripard@bootlin.com \
    --cc=mironov.ivan@gmail.com \
    --cc=sean@poorly.run \
    /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).