Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <36f682cf61b353441ff41cf651650f7117247052.camel@gmail.com>
Subject: Re: [PATCH v1 1/2] drm/fb-helper: Bring back workaround for bugs of
 SDL 1.2
From:   Ivan Mironov <mironov.ivan@gmail.com>
To:     Daniel Vetter <daniel@ffwll.ch>
Cc:     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>,
        stable@vger.kernel.org
Date:   Tue, 08 Jan 2019 12:26:52 +0500
In-Reply-To: <20190107100842.GZ21184@phenom.ffwll.local>
References: <20181227231308.16904-1-mironov.ivan@gmail.com>
         <20181227231308.16904-2-mironov.ivan@gmail.com>
         <20181228121549.GU9058@dvetter-linux.ger.corp.intel.com>
         <e4754c8dab11b1f8b61a7fb2aae690b979cc7209.camel@gmail.com>
         <20190107100842.GZ21184@phenom.ffwll.local>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.3 (3.30.3-1.fc29) 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2019-01-07 at 11:08 +0100, Daniel Vetter wrote:
> > > > @@ -1654,6 +1712,40 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * Workaround for SDL 1.2, which is known to be setting all pixel format
> > > > +	 * fields values to zero in some cases. We treat this situation as a
> > > > +	 * kind of "use some reasonable autodetected values".
> > > > +	 */
> > > > +	if (!var->red.offset     && !var->green.offset    &&
> > > > +	    !var->blue.offset    && !var->transp.offset   &&
> > > > +	    !var->red.length     && !var->green.length    &&
> > > > +	    !var->blue.length    && !var->transp.length   &&
> > > > +	    !var->red.msb_right  && !var->green.msb_right &&
> > > > +	    !var->blue.msb_right && !var->transp.msb_right) {
> > > > +		u8 depth;
> > > > +
> > > > +		/*
> > > > +		 * There is no way to guess the right value for depth when
> > > > +		 * bpp is 16 or 32. So we just restore the behaviour previously
> > > > +		 * introduced here by commit . In fact, this was
> > > > +		 * implemented even earlier in various device drivers.
> > > > +		 */
> > > > +		switch (var->bits_per_pixel) {
> > > > +		case 16:
> > > > +			depth = 15;
> > > > +			break;
> > > > +		case 32:
> > > > +			depth = 24;
> > > > +			break;
> > > > +		default:
> > > > +			depth = var->bits_per_pixel;
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		drm_fb_helper_fill_pixel_fmt(var, depth);
> > > 
> > > Please use fb->format->depth here instead of guessing.
> > > -Daniel
> > 
> > I do not think that this is the right way.
> > 
> > Without guessing, if SDL1 makes a request with bits_per_pixel == 16
> > (for example) and current fb->format->depth == 24, ioctl() will succeed
> > while actual pixel format will remain the same. As a result, SDL1 will
> > display garbage because of invalid pixel format.
> > 
> > Or am I missing something here?
> 
> This is supposed to be the case where SDL didn't set any of this stuff.
> Guess is definitely not what we want to do, we should use the actual
> depth, which is stored in fb->format->depth. Older code did guess, but we
> fixed that, and shouldn't reintroduce that guess game.
> -Daniel
> 

Done. See the v2 patch series.