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

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

[Rohit Visavalia]

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@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

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rohit Visavalia]

Gentle reminder.

+ Ilia Mirkin, +Emil Velikov.

Thanks & Regards,
Rohit 

> -----Original Message-----
> From: Rohit Visavalia [mailto:rohit.visavalia@xilinx.com]
> Sent: Tuesday, February 25, 2020 3:08 PM
> To: dri-devel@lists.freedesktop.org
> Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@xilinx.com>; Rohit
> Visavalia <RVISAVAL@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@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Devarsh Thakkar]

Hi Rohit,

This makes sense to me as gamma was implemented as optional property.
Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com>"

@emil.velikov@collabora.com, @imirkin@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@xilinx.com>; dri-devel@lists.freedesktop.org;
> imirkin@alum.mit.edu; emil.velikov@collabora.com
> Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@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@xilinx.com]
> > Sent: Tuesday, February 25, 2020 3:08 PM
> > To: dri-devel@lists.freedesktop.org
> > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@xilinx.com>; Rohit
> > Visavalia <RVISAVAL@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@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ilia Mirkin]

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@xilinx.com> wrote:
>
> Hi Rohit,
>
> This makes sense to me as gamma was implemented as optional property.
> Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com>"
>
> @emil.velikov@collabora.com, @imirkin@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@xilinx.com>; dri-devel@lists.freedesktop.org;
> > imirkin@alum.mit.edu; emil.velikov@collabora.com
> > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> > Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@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@xilinx.com]
> > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > To: dri-devel@lists.freedesktop.org
> > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@xilinx.com>; Rohit
> > > Visavalia <RVISAVAL@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@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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rohit Visavalia]

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@alum.mit.edu]
> Sent: Tuesday, March 3, 2020 7:08 PM
> To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> Cc: Rohit Visavalia <RVISAVAL@xilinx.com>; dri-devel@lists.freedesktop.org;
> emil.velikov@collabora.com; Ville Syrjälä <ville.syrjala@linux.intel.com>; Hyun
> Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> <VARUNKUM@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@xilinx.com>
> wrote:
> >
> > Hi Rohit,
> >
> > This makes sense to me as gamma was implemented as optional property.
> > Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com>"
> >
> > @emil.velikov@collabora.com, @imirkin@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@xilinx.com>;
> > > dri-devel@lists.freedesktop.org; imirkin@alum.mit.edu;
> > > emil.velikov@collabora.com
> > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@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@xilinx.com]
> > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > To: dri-devel@lists.freedesktop.org
> > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@xilinx.com>;
> > > > Rohit Visavalia <RVISAVAL@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@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
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ilia Mirkin]

[-- Attachment #1.1: Type: text/plain, Size: 4591 bytes --]

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@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@alum.mit.edu]
> > Sent: Tuesday, March 3, 2020 7:08 PM
> > To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>;
> dri-devel@lists.freedesktop.org;
> > emil.velikov@collabora.com; Ville Syrjälä <ville.syrjala@linux.intel.com>;
> Hyun
> > Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> > Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > <VARUNKUM@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@xilinx.com>
> > wrote:
> > >
> > > Hi Rohit,
> > >
> > > This makes sense to me as gamma was implemented as optional property.
> > > Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com>"
> > >
> > > @emil.velikov@collabora.com, @imirkin@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@xilinx.com>;
> > > > dri-devel@lists.freedesktop.org; imirkin@alum.mit.edu;
> > > > emil.velikov@collabora.com
> > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@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@xilinx.com]
> > > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > > To: dri-devel@lists.freedesktop.org
> > > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@xilinx.com>;
> > > > > Rohit Visavalia <RVISAVAL@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@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
> > >
>

[-- Attachment #1.2: Type: text/html, Size: 8429 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rohit Visavalia]

[-- Attachment #1.1: Type: text/plain, Size: 6374 bytes --]

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@RG16).

What is the requirement for setting gamma drmModeCrtcSetGamma() if user has not asked ?

Thanks
Rohit

From: Ilia Mirkin [mailto:imirkin@alum.mit.edu]
Sent: Thursday, March 12, 2020 3:49 PM
To: Rohit Visavalia <RVISAVAL@xilinx.com>
Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; dri-devel <dri-devel@lists.freedesktop.org>; emil.velikov@collabora.com; Ville Syrjälä <ville.syrjala@linux.intel.com>; Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa <VARUNKUM@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@xilinx.com<mailto:RVISAVAL@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@alum.mit.edu<mailto:imirkin@alum.mit.edu>]
> Sent: Tuesday, March 3, 2020 7:08 PM
> To: Devarsh Thakkar <DEVARSHT@xilinx.com<mailto:DEVARSHT@xilinx.com>>
> Cc: Rohit Visavalia <RVISAVAL@xilinx.com<mailto:RVISAVAL@xilinx.com>>; dri-devel@lists.freedesktop.org<mailto:dri-devel@lists.freedesktop.org>;
> emil.velikov@collabora.com<mailto:emil.velikov@collabora.com>; Ville Syrjälä <ville.syrjala@linux.intel.com<mailto:ville.syrjala@linux.intel.com>>; Hyun
> Kwon <hyunk@xilinx.com<mailto:hyunk@xilinx.com>>; Ranganathan Sk <rsk@xilinx.com<mailto:rsk@xilinx.com>>; Dhaval
> Rajeshbhai Shah <dshah@xilinx.com<mailto:dshah@xilinx.com>>; Varunkumar Allagadapa
> <VARUNKUM@xilinx.com<mailto:VARUNKUM@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@xilinx.com<mailto:DEVARSHT@xilinx.com>>
> wrote:
> >
> > Hi Rohit,
> >
> > This makes sense to me as gamma was implemented as optional property.
> > Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com<mailto:devarsh.thakkar@xilinx.com>>"
> >
> > @emil.velikov@collabora.com<mailto:emil.velikov@collabora.com>, @imirkin@alum.mit.edu<mailto:imirkin@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@xilinx.com<mailto:RVISAVAL@xilinx.com>>;
> > > dri-devel@lists.freedesktop.org<mailto:dri-devel@lists.freedesktop.org>; imirkin@alum.mit.edu<mailto:imirkin@alum.mit.edu>;
> > > emil.velikov@collabora.com<mailto:emil.velikov@collabora.com>
> > > Cc: Hyun Kwon <hyunk@xilinx.com<mailto:hyunk@xilinx.com>>; Ranganathan Sk <rsk@xilinx.com<mailto:rsk@xilinx.com>>;
> > > Dhaval Rajeshbhai Shah <dshah@xilinx.com<mailto:dshah@xilinx.com>>; Varunkumar Allagadapa
> > > <VARUNKUM@xilinx.com<mailto:VARUNKUM@xilinx.com>>; Devarsh Thakkar <DEVARSHT@xilinx.com<mailto:DEVARSHT@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@xilinx.com<mailto:rohit.visavalia@xilinx.com>]
> > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > To: dri-devel@lists.freedesktop.org<mailto:dri-devel@lists.freedesktop.org>
> > > > Cc: Hyun Kwon <hyunk@xilinx.com<mailto:hyunk@xilinx.com>>; Ranganathan Sk <rsk@xilinx.com<mailto:rsk@xilinx.com>>;
> > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com<mailto:dshah@xilinx.com>>; Varunkumar Allagadapa
> > > > <VARUNKUM@xilinx.com<mailto:VARUNKUM@xilinx.com>>; Devarsh Thakkar <DEVARSHT@xilinx.com<mailto:DEVARSHT@xilinx.com>>;
> > > > Rohit Visavalia <RVISAVAL@xilinx.com<mailto:RVISAVAL@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@xilinx.com<mailto:rohit.visavalia@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
> >

[-- Attachment #1.2: Type: text/html, Size: 12823 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ilia Mirkin]

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@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@RG16).
>
>
>
> What is the requirement for setting gamma drmModeCrtcSetGamma() if user has not asked ?
>
>
>
> Thanks
> Rohit
>
>
>
> From: Ilia Mirkin [mailto:imirkin@alum.mit.edu]
> Sent: Thursday, March 12, 2020 3:49 PM
> To: Rohit Visavalia <RVISAVAL@xilinx.com>
> Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; dri-devel <dri-devel@lists.freedesktop.org>; emil.velikov@collabora.com; Ville Syrjälä <ville.syrjala@linux.intel.com>; Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa <VARUNKUM@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@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@alum.mit.edu]
> > Sent: Tuesday, March 3, 2020 7:08 PM
> > To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>; dri-devel@lists.freedesktop.org;
> > emil.velikov@collabora.com; Ville Syrjälä <ville.syrjala@linux.intel.com>; Hyun
> > Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> > Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > <VARUNKUM@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@xilinx.com>
> > wrote:
> > >
> > > Hi Rohit,
> > >
> > > This makes sense to me as gamma was implemented as optional property.
> > > Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com>"
> > >
> > > @emil.velikov@collabora.com, @imirkin@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@xilinx.com>;
> > > > dri-devel@lists.freedesktop.org; imirkin@alum.mit.edu;
> > > > emil.velikov@collabora.com
> > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@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@xilinx.com]
> > > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > > To: dri-devel@lists.freedesktop.org
> > > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>;
> > > > > Dhaval Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > > > <VARUNKUM@xilinx.com>; Devarsh Thakkar <DEVARSHT@xilinx.com>;
> > > > > Rohit Visavalia <RVISAVAL@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@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
> > >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [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@alum.mit.edu]
> Sent: Friday, March 13, 2020 8:17 PM
> To: Rohit Visavalia <RVISAVAL@xilinx.com>
> Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; dri-devel <dri-
> devel@lists.freedesktop.org>; emil.velikov@collabora.com; Ville Syrjälä
> <ville.syrjala@linux.intel.com>; Hyun Kwon <hyunk@xilinx.com>; Ranganathan
> Sk <rsk@xilinx.com>; Dhaval Rajeshbhai Shah <dshah@xilinx.com>;
> Varunkumar Allagadapa <VARUNKUM@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@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@RG16).
> >
> >
> >
> > What is the requirement for setting gamma drmModeCrtcSetGamma() if user
> has not asked ?
> >
> >
> >
> > Thanks
> > Rohit
> >
> >
> >
> > From: Ilia Mirkin [mailto:imirkin@alum.mit.edu]
> > Sent: Thursday, March 12, 2020 3:49 PM
> > To: Rohit Visavalia <RVISAVAL@xilinx.com>
> > Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; dri-devel
> > <dri-devel@lists.freedesktop.org>; emil.velikov@collabora.com; Ville
> > Syrjälä <ville.syrjala@linux.intel.com>; Hyun Kwon <hyunk@xilinx.com>;
> > Ranganathan Sk <rsk@xilinx.com>; Dhaval Rajeshbhai Shah
> > <dshah@xilinx.com>; Varunkumar Allagadapa <VARUNKUM@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@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@alum.mit.edu]
> > > Sent: Tuesday, March 3, 2020 7:08 PM
> > > To: Devarsh Thakkar <DEVARSHT@xilinx.com>
> > > Cc: Rohit Visavalia <RVISAVAL@xilinx.com>;
> > > dri-devel@lists.freedesktop.org; emil.velikov@collabora.com; Ville
> > > Syrjälä <ville.syrjala@linux.intel.com>; Hyun Kwon
> > > <hyunk@xilinx.com>; Ranganathan Sk <rsk@xilinx.com>; Dhaval
> > > Rajeshbhai Shah <dshah@xilinx.com>; Varunkumar Allagadapa
> > > <VARUNKUM@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@xilinx.com>
> > > wrote:
> > > >
> > > > Hi Rohit,
> > > >
> > > > This makes sense to me as gamma was implemented as optional
> property.
> > > > Reviewed-By: "Devarsh Thakkar <devarsh.thakkar@xilinx.com>"
> > > >
> > > > @emil.velikov@collabora.com, @imirkin@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@xilinx.com>;
> > > > > dri-devel@lists.freedesktop.org; imirkin@alum.mit.edu;
> > > > > emil.velikov@collabora.com
> > > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk
> > > > > <rsk@xilinx.com>; Dhaval Rajeshbhai Shah <dshah@xilinx.com>;
> > > > > Varunkumar Allagadapa <VARUNKUM@xilinx.com>; Devarsh Thakkar
> > > > > <DEVARSHT@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@xilinx.com]
> > > > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > > > To: dri-devel@lists.freedesktop.org
> > > > > > Cc: Hyun Kwon <hyunk@xilinx.com>; Ranganathan Sk
> > > > > > <rsk@xilinx.com>; Dhaval Rajeshbhai Shah <dshah@xilinx.com>;
> > > > > > Varunkumar Allagadapa <VARUNKUM@xilinx.com>; Devarsh Thakkar
> > > > > > <DEVARSHT@xilinx.com>; Rohit Visavalia <RVISAVAL@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@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
> > > >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel