[PATCH libdrm] modetest: call drmModeCrtcSetGamma() only if add_property_optional returns true

[Rohit Visavalia]

Hi Ilia Mirkin,

Thanks for the comments.

I have sent new patch for review, can you please check ?

Thanks,
Rohit

> -----Original Message-----
> From: Ilia Mirkin [mailto:imirkin at alum.mit.edu]
> Sent: Friday, March 13, 2020 8:17 PM
> To: Rohit Visavalia <RVISAVAL at xilinx.com>
> Cc: Devarsh Thakkar <DEVARSHT at xilinx.com>; dri-devel <dri-
> devel at lists.freedesktop.org>; emil.velikov at collabora.com; Ville Syrjälä
> <ville.syrjala at linux.intel.com>; Hyun Kwon <hyunk at xilinx.com>; Ranganathan
> Sk <rsk at xilinx.com>; Dhaval Rajeshbhai Shah <dshah at xilinx.com>;
> Varunkumar Allagadapa <VARUNKUM at xilinx.com>
> Subject: Re: [PATCH libdrm] modetest: call drmModeCrtcSetGamma() only if
> add_property_optional returns true
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> It's to restore the gamma ramp to be the default linear one. Let's say that the
> driver doesn't have the GAMMA_LUT property support, and then you want to
> modeset with C8 (indexed) format. That means that modeset has to set the
> LUT to make the indexed lookups work (which it does, it all works, you
> celebrate). Then you want to run modeset with e.g.
> XR24, and the colors are all black! The LUT is persistent across modesets, so it
> has to reset the ramp to linear.
> 
> You could condition calling set_gamma on crtc->gamma_size > 0, I think
> -- it didn't occur to me that there would be drivers that didn't support a LUT.
> 
> On Fri, Mar 13, 2020 at 6:08 AM Rohit Visavalia <RVISAVAL at xilinx.com> wrote:
> >
> > Hi Ilia Mirkin,
> >
> >
> >
> > But it should not go for setting gamma(drmModeCrtcSetGamma) as user has
> not asked to do so in just simple mode set command(modetest -M <module> -
> s 42:3840x2160 at RG16).
> >
> >
> >
> > What is the requirement for setting gamma drmModeCrtcSetGamma() if user
> has not asked ?
> >
> >
> >
> > Thanks
> > Rohit
> >
> >
> >
> > From: Ilia Mirkin [mailto:imirkin at alum.mit.edu]
> > Sent: Thursday, March 12, 2020 3:49 PM
> > To: Rohit Visavalia <RVISAVAL at xilinx.com>
> > Cc: Devarsh Thakkar <DEVARSHT at xilinx.com>; dri-devel
> > <dri-devel at lists.freedesktop.org>; emil.velikov at collabora.com; Ville
> > Syrjälä <ville.syrjala at linux.intel.com>; Hyun Kwon <hyunk at xilinx.com>;
> > Ranganathan Sk <rsk at xilinx.com>; Dhaval Rajeshbhai Shah
> > <dshah at xilinx.com>; Varunkumar Allagadapa <VARUNKUM at xilinx.com>
> > Subject: Re: [PATCH libdrm] modetest: call drmModeCrtcSetGamma() only
> > if add_property_optional returns true
> >
> >
> >
> > CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> >
> >
> >
> > Hm. I'm not sure offhand how to check if drmModeCrtcSetGamma is
> supported. I guess you could check if gamma size > 0 or something?
> >
> >
> >
> > On Thu, Mar 12, 2020, 02:39 Rohit Visavalia <RVISAVAL at xilinx.com> wrote:
> >
> > Hi Ilia Mirkin,
> >
> > Thanks for the review.
> >
> > By old-fashioned way you mean to say using drmModeCrtcSetGamma()? If
> yes then, it shows error as "failed to set gamma: Function no implemented" if
> any platform specific drm has no gamma property implemented.
> >
> > Current code shows error while running modetest for Xilinx drm as it doesn't
> supports gamma property and ideally it should not show error as gamma is
> optional property, so it doesn't serve the purpose of optional property.
> >
> > Please correct me if I am missing anything.
> >
> > Thanks
> > Rohit
> >
> > > -----Original Message-----
> > > From: Ilia Mirkin [mailto:imirkin at alum.mit.edu]
> > > Sent: Tuesday, March 3, 2020 7:08 PM
> > > To: Devarsh Thakkar <DEVARSHT at xilinx.com>
> > > Cc: Rohit Visavalia <RVISAVAL at xilinx.com>;
> > > dri-devel at lists.freedesktop.org; emil.velikov at collabora.com; Ville
> > > Syrjälä <ville.syrjala at linux.intel.com>; Hyun Kwon
> > > <hyunk at xilinx.com>; Ranganathan Sk <rsk at xilinx.com>; Dhaval
> > > Rajeshbhai Shah <dshah at xilinx.com>; Varunkumar Allagadapa
> > > <VARUNKUM at xilinx.com>
> > > Subject: Re: [PATCH libdrm] modetest: call drmModeCrtcSetGamma()
> > > only if add_property_optional returns true
> > >
> > > EXTERNAL EMAIL
> > >
> > > Pretty sure the current code is right. If the GAMMA_LUT property
> > > can't be set, it tries to set gamma the old-fashioned way.
> > >
> > > On Tue, Mar 3, 2020 at 8:12 AM Devarsh Thakkar <DEVARSHT at xilinx.com>
> > > wrote:
> > > >
> > > > Hi Rohit,
> > > >
> > > > This makes sense to me as gamma was implemented as optional
> property.
> > > > Reviewed-By: "Devarsh Thakkar <devarsh.thakkar at xilinx.com>"
> > > >
> > > > @emil.velikov at collabora.com, @imirkin at alum.mit.edu, @Ville
> > > > Syrjälä,
> > > Could you please ack and help merge this patch if it also look good to you ?
> > > >
> > > > Regards,
> > > > Devarsh
> > > >
> > > > > -----Original Message-----
> > > > > From: Rohit Visavalia
> > > > > Sent: 27 February 2020 00:40
> > > > > To: Rohit Visavalia <RVISAVAL at xilinx.com>;
> > > > > dri-devel at lists.freedesktop.org; imirkin at alum.mit.edu;
> > > > > emil.velikov at collabora.com
> > > > > Cc: Hyun Kwon <hyunk at xilinx.com>; Ranganathan Sk
> > > > > <rsk at xilinx.com>; Dhaval Rajeshbhai Shah <dshah at xilinx.com>;
> > > > > Varunkumar Allagadapa <VARUNKUM at xilinx.com>; Devarsh Thakkar
> > > > > <DEVARSHT at xilinx.com>
> > > > > Subject: RE: [PATCH libdrm] modetest: call drmModeCrtcSetGamma()
> > > > > only if add_property_optional returns true
> > > > >
> > > > > Gentle reminder.
> > > > >
> > > > > + Ilia Mirkin, +Emil Velikov.
> > > > >
> > > > > Thanks & Regards,
> > > > > Rohit
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Rohit Visavalia [mailto:rohit.visavalia at xilinx.com]
> > > > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > > > To: dri-devel at lists.freedesktop.org
> > > > > > Cc: Hyun Kwon <hyunk at xilinx.com>; Ranganathan Sk
> > > > > > <rsk at xilinx.com>; Dhaval Rajeshbhai Shah <dshah at xilinx.com>;
> > > > > > Varunkumar Allagadapa <VARUNKUM at xilinx.com>; Devarsh Thakkar
> > > > > > <DEVARSHT at xilinx.com>; Rohit Visavalia <RVISAVAL at xilinx.com>
> > > > > > Subject: [PATCH libdrm] modetest: call drmModeCrtcSetGamma()
> > > > > > only if add_property_optional returns true
> > > > > >
> > > > > > gamma is a optional property then also it prints error
> > > > > > message, so set gamma only if add_property_optional() returns true.
> > > > > >
> > > > > > Signed-off-by: Rohit Visavalia <rohit.visavalia at xilinx.com>
> > > > > > ---
> > > > > >  tests/modetest/modetest.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/tests/modetest/modetest.c
> > > > > > b/tests/modetest/modetest.c index b907ab3..379b9ea 100644
> > > > > > --- a/tests/modetest/modetest.c
> > > > > > +++ b/tests/modetest/modetest.c
> > > > > > @@ -1138,7 +1138,7 @@ static void set_gamma(struct device
> > > > > > *dev, unsigned crtc_id, unsigned fourcc)
> > > > > >
> > > > > >     add_property_optional(dev, crtc_id, "DEGAMMA_LUT", 0);
> > > > > >     add_property_optional(dev, crtc_id, "CTM", 0);
> > > > > > -   if (!add_property_optional(dev, crtc_id, "GAMMA_LUT", blob_id)) {
> > > > > > +   if (add_property_optional(dev, crtc_id, "GAMMA_LUT",
> > > > > > + blob_id)) {
> > > > > >             uint16_t r[256], g[256], b[256];
> > > > > >
> > > > > >             for (i = 0; i < 256; i++) {
> > > > > > --
> > > > > > 2.7.4
> > > >