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;
Date:   Fri, 28 Dec 2018 13:02:03 +0100
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: [RFC PATCH 1/2] drm/fb-helper: Bring back workaround for bugs of
 SDL 1.2
Message-ID: <20181228120203.GQ9058@dvetter-linux.ger.corp.intel.com>
Mail-Followup-To: Ivan Mironov <mironov.ivan@gmail.com>,
        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>
References: <20181226121124.1632-1-mironov.ivan@gmail.com>
 <20181226121124.1632-2-mironov.ivan@gmail.com>
 <20181227120046.GC9058@dvetter-linux.ger.corp.intel.com>
 <00547df7d87c7fe6d8c842d5b29a0cd6809cdf5d.camel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <00547df7d87c7fe6d8c842d5b29a0cd6809cdf5d.camel@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Dec 28, 2018 at 04:26:56AM +0500, Ivan Mironov wrote:
> On Thu, 2018-12-27 at 13:00 +0100, Daniel Vetter wrote: 
> > > +	/*
> > > +	 * 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 785b93ef8c309. 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;
> > > +		}
> > 
> > The guesswork here looks fishy. We should still have the drm-side format,
> > and should use that.
> 
> This existed for a very long time until problematic commit was applied.
> And there is a clear evidence that it was actually used by
> applications.

I'm not against guessing this stuff, but we know have much better format
handling code than when this code was originally written. I just want to
use that (like it's used everywhere else in this file now). fb->format
gives you the right depth, no guessing needed at all.

And if you guess wrong here, you'll fail in these format checks later on.
-Daniel
> 
> > Otherwise the patches look good I think, but they
> > need a Fixes: tag and cc: stable so backporters know what to do with
> > these.
> > 
> 
> I added "cc: stable" into the regression fix. Also added more info into
> the commit messages. See the PATCH v1 in the mailing list.
> 
> Thanks.
> 
> 
> 

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