[PATCH v3] drm/fsl-dcu: Implement gamma_lut atomic crtc properties

[Stefan Agner]

On 2016-09-25 23:04, Meng Yi wrote:
>> On Wed, Sep 21, 2016 at 11:10:11AM -0700, Stefan Agner wrote:
>> > On 2016-09-13 01:49, Meng Yi wrote:
>> > >> > diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
>> > >> > b/drivers/gpu/drm/fsl-dcu/Kconfig index 14a72c4..f9c76b1 100644
>> > >> > --- a/drivers/gpu/drm/fsl-dcu/Kconfig
>> > >> > +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
>> > >> > @@ -11,3 +11,9 @@ config DRM_FSL_DCU
>> > >> >  	help
>> > >> >  	  Choose this option if you have an Freescale DCU chipset.
>> > >> >  	  If M is selected the module will be called fsl-dcu-drm.
>> > >> > +
>> > >> > +config DRM_FSL_DCU_GAMMA
>> > >> > +	bool "Gamma Correction Support for NXP/Freescale DCU"
>> > >> > +	depends on DRM_FSL_DCU
>> > >> > +	help
>> > >> > +	  Enable support for gamma correction.
>> > >>
>> > >> What is the reason for making this a configuration option? Are
>> > >> there implementation without support for the Gamma tables?
>> > >>
>> > > When gamma correction is enabled, the color won't display normally
>> > > since The gamma tables are not filled with correct data. So I give a
>> > > choice to not using The gamma correction when you don't want to use
>> > > it.
>> > >
>> > > Should I remove this configuration?
>> >
>> > Yeah making this a compile time configuration seems wrong to me.
>> >
>> > I guess we should fill the table with a reasonable default then. The
>> > omapdrm driver seems to do something similar.
>>
>> Yup Kconfig is definitely not the correct way to fix a setup problem in your
>> driver. Note that even when you enable gamma lut userspace is allowed to set
>> a NULL table, and in that case you need to either disable the gamma lut, or fill it
>> with a 1:1 mapping (if you hw doesn't have a disable flag). At boot-up the table
>> will also be NULL (except when you have special setup code), and if that results
>> in a black screen your driver is buggy. Papering over a driver bug with Kconfig is
>> definitely not ok.
> 
> How about initialize the tables with static array(such as gmma = 2.2, etc)

The hardware allows to disable gamma, so I would rather prefer to only
enable it when a valid table is set, and disable gamma at startup and
when a NULL table is set. This also makes sure what we get the least
interference with what we are doing now.

--
Stefan