* [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode
@ 2019-12-02 11:27 Devarsh Thakkar
2019-12-02 17:12 ` Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Devarsh Thakkar @ 2019-12-02 11:27 UTC (permalink / raw)
To: dri-devel, ville.syrjala; +Cc: rsk, vcu-team, dshah, devarsh.thakkar, varunkum
Add function to derive floating value of vertical
refresh rate from drm mode using pixel clock,
horizontal total size and vertical total size.
Use this function to find suitable mode having vrefresh
value which is matching with user provided vrefresh value.
If user doesn't provide any vrefresh value in args then
update vertical refresh rate value in pipe args using this
function.
Also use this function for printing floating vrefresh while
dumping all available modes.
This will give more accurate picture to user for available modes
differentiated by floating vertical refresh rate and help user
select more appropriate mode using suitable refresh rate value.
V3: Change name of function used to derive refresh rate.
V2: 1) Don't use inline function for deriving refresh rate from mode.
2) If requested mode not found, print refresh rate only
if user had provided it in args.
Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
---
tests/modetest/modetest.c | 40 +++++++++++++++++++++++++++-------------
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index b4edfcb..19ce20f 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val)
return (int64_t)*((int64_t *)&val);
}
+static float mode_vrefresh(drmModeModeInfo *mode)
+{
+ return mode->clock * 1000.00
+ / (mode->htotal * mode->vtotal);
+}
+
#define bit_name_fn(res) \
const char * res##_str(int type) { \
unsigned int i; \
@@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev)
static void dump_mode(drmModeModeInfo *mode)
{
- printf(" %s %d %d %d %d %d %d %d %d %d %d",
+ printf(" %s %.2f %d %d %d %d %d %d %d %d %d",
mode->name,
- mode->vrefresh,
+ mode_vrefresh(mode),
mode->hdisplay,
mode->hsync_start,
mode->hsync_end,
@@ -823,12 +829,11 @@ struct plane_arg {
static drmModeModeInfo *
connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
- const float vrefresh)
+ float *vrefresh)
{
drmModeConnector *connector;
drmModeModeInfo *mode;
int i;
- float mode_vrefresh;
connector = get_connector_by_id(dev, con_id);
if (!connector || !connector->count_modes)
@@ -837,16 +842,19 @@ connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
for (i = 0; i < connector->count_modes; i++) {
mode = &connector->modes[i];
if (!strcmp(mode->name, mode_str)) {
- /* If the vertical refresh frequency is not specified then return the
- * first mode that match with the name. Else, return the mode that match
- * the name and the specified vertical refresh frequency.
+ /* If the vertical refresh frequency is not specified
+ * then return the first mode that match with the name
+ * and update corresponding vrefresh in pipe_arg.
+ * Else, return the mode that match the name and
+ * the specified vertical refresh frequency.
*/
- mode_vrefresh = mode->clock * 1000.00
- / (mode->htotal * mode->vtotal);
- if (vrefresh == 0)
+ if (*vrefresh == 0) {
+ *vrefresh = mode_vrefresh(mode);
return mode;
- else if (fabs(mode_vrefresh - vrefresh) < 0.005)
+ } else if (fabs(mode_vrefresh(mode)
+ - *vrefresh) < 0.005) {
return mode;
+ }
}
}
@@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct device *dev, struct pipe_arg *pipe)
for (i = 0; i < (int)pipe->num_cons; i++) {
mode = connector_find_mode(dev, pipe->con_ids[i],
- pipe->mode_str, pipe->vrefresh);
+ pipe->mode_str, &pipe->vrefresh);
if (mode == NULL) {
- fprintf(stderr,
+ if (pipe->vrefresh)
+ fprintf(stderr,
+ "failed to find mode "
+ "\"%s-%.2fHz\" for connector %s\n",
+ pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
+ else
+ fprintf(stderr,
"failed to find mode \"%s\" for connector %s\n",
pipe->mode_str, pipe->cons[i]);
return -EINVAL;
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode
2019-12-02 11:27 [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode Devarsh Thakkar
@ 2019-12-02 17:12 ` Ville Syrjälä
2019-12-02 17:22 ` Neil Armstrong
0 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2019-12-02 17:12 UTC (permalink / raw)
To: Devarsh Thakkar; +Cc: rsk, vcu-team, dshah, dri-devel, varunkum
On Mon, Dec 02, 2019 at 03:27:51AM -0800, Devarsh Thakkar wrote:
> Add function to derive floating value of vertical
> refresh rate from drm mode using pixel clock,
> horizontal total size and vertical total size.
>
> Use this function to find suitable mode having vrefresh
> value which is matching with user provided vrefresh value.
>
> If user doesn't provide any vrefresh value in args then
> update vertical refresh rate value in pipe args using this
> function.
>
> Also use this function for printing floating vrefresh while
> dumping all available modes.
>
> This will give more accurate picture to user for available modes
> differentiated by floating vertical refresh rate and help user
> select more appropriate mode using suitable refresh rate value.
>
> V3: Change name of function used to derive refresh rate.
> V2: 1) Don't use inline function for deriving refresh rate from mode.
> 2) If requested mode not found, print refresh rate only
> if user had provided it in args.
>
> Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
> ---
> tests/modetest/modetest.c | 40 +++++++++++++++++++++++++++-------------
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> index b4edfcb..19ce20f 100644
> --- a/tests/modetest/modetest.c
> +++ b/tests/modetest/modetest.c
> @@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val)
> return (int64_t)*((int64_t *)&val);
> }
>
> +static float mode_vrefresh(drmModeModeInfo *mode)
> +{
> + return mode->clock * 1000.00
> + / (mode->htotal * mode->vtotal);
> +}
> +
> #define bit_name_fn(res) \
> const char * res##_str(int type) { \
> unsigned int i; \
> @@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev)
>
> static void dump_mode(drmModeModeInfo *mode)
> {
> - printf(" %s %d %d %d %d %d %d %d %d %d %d",
> + printf(" %s %.2f %d %d %d %d %d %d %d %d %d",
> mode->name,
> - mode->vrefresh,
> + mode_vrefresh(mode),
> mode->hdisplay,
> mode->hsync_start,
> mode->hsync_end,
> @@ -823,12 +829,11 @@ struct plane_arg {
>
> static drmModeModeInfo *
> connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
> - const float vrefresh)
> + float *vrefresh)
This change still looks pointless.
> {
> drmModeConnector *connector;
> drmModeModeInfo *mode;
> int i;
> - float mode_vrefresh;
>
> connector = get_connector_by_id(dev, con_id);
> if (!connector || !connector->count_modes)
> @@ -837,16 +842,19 @@ connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
> for (i = 0; i < connector->count_modes; i++) {
> mode = &connector->modes[i];
> if (!strcmp(mode->name, mode_str)) {
> - /* If the vertical refresh frequency is not specified then return the
> - * first mode that match with the name. Else, return the mode that match
> - * the name and the specified vertical refresh frequency.
> + /* If the vertical refresh frequency is not specified
> + * then return the first mode that match with the name
> + * and update corresponding vrefresh in pipe_arg.
> + * Else, return the mode that match the name and
> + * the specified vertical refresh frequency.
> */
> - mode_vrefresh = mode->clock * 1000.00
> - / (mode->htotal * mode->vtotal);
> - if (vrefresh == 0)
> + if (*vrefresh == 0) {
> + *vrefresh = mode_vrefresh(mode);
> return mode;
> - else if (fabs(mode_vrefresh - vrefresh) < 0.005)
> + } else if (fabs(mode_vrefresh(mode)
> + - *vrefresh) < 0.005) {
> return mode;
> + }
> }
> }
>
> @@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct device *dev, struct pipe_arg *pipe)
>
> for (i = 0; i < (int)pipe->num_cons; i++) {
> mode = connector_find_mode(dev, pipe->con_ids[i],
> - pipe->mode_str, pipe->vrefresh);
> + pipe->mode_str, &pipe->vrefresh);
> if (mode == NULL) {
> - fprintf(stderr,
> + if (pipe->vrefresh)
> + fprintf(stderr,
> + "failed to find mode "
> + "\"%s-%.2fHz\" for connector %s\n",
> + pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
> + else
> + fprintf(stderr,
> "failed to find mode \"%s\" for connector %s\n",
> pipe->mode_str, pipe->cons[i]);
> return -EINVAL;
> --
> 2.7.4
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode
2019-12-02 17:12 ` Ville Syrjälä
@ 2019-12-02 17:22 ` Neil Armstrong
2019-12-02 17:42 ` Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Neil Armstrong @ 2019-12-02 17:22 UTC (permalink / raw)
To: Ville Syrjälä, Devarsh Thakkar
Cc: rsk, varunkum, vcu-team, dri-devel, dshah
On 02/12/2019 18:12, Ville Syrjälä wrote:
> On Mon, Dec 02, 2019 at 03:27:51AM -0800, Devarsh Thakkar wrote:
>> Add function to derive floating value of vertical
>> refresh rate from drm mode using pixel clock,
>> horizontal total size and vertical total size.
>>
>> Use this function to find suitable mode having vrefresh
>> value which is matching with user provided vrefresh value.
>>
>> If user doesn't provide any vrefresh value in args then
>> update vertical refresh rate value in pipe args using this
>> function.
>>
>> Also use this function for printing floating vrefresh while
>> dumping all available modes.
>>
>> This will give more accurate picture to user for available modes
>> differentiated by floating vertical refresh rate and help user
>> select more appropriate mode using suitable refresh rate value.
>>
>> V3: Change name of function used to derive refresh rate.
>> V2: 1) Don't use inline function for deriving refresh rate from mode.
>> 2) If requested mode not found, print refresh rate only
>> if user had provided it in args.
>>
>> Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
>> ---
>> tests/modetest/modetest.c | 40 +++++++++++++++++++++++++++-------------
>> 1 file changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
>> index b4edfcb..19ce20f 100644
>> --- a/tests/modetest/modetest.c
>> +++ b/tests/modetest/modetest.c
>> @@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val)
>> return (int64_t)*((int64_t *)&val);
>> }
>>
>> +static float mode_vrefresh(drmModeModeInfo *mode)
>> +{
>> + return mode->clock * 1000.00
>> + / (mode->htotal * mode->vtotal);
>> +}
>> +
>> #define bit_name_fn(res) \
>> const char * res##_str(int type) { \
>> unsigned int i; \
>> @@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev)
>>
>> static void dump_mode(drmModeModeInfo *mode)
>> {
>> - printf(" %s %d %d %d %d %d %d %d %d %d %d",
>> + printf(" %s %.2f %d %d %d %d %d %d %d %d %d",
>> mode->name,
>> - mode->vrefresh,
>> + mode_vrefresh(mode),
>> mode->hdisplay,
>> mode->hsync_start,
>> mode->hsync_end,
>> @@ -823,12 +829,11 @@ struct plane_arg {
>>
>> static drmModeModeInfo *
>> connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
>> - const float vrefresh)
>> + float *vrefresh)
>
> This change still looks pointless.
Without this, you cannot set 1000/1001 CEA alternate clock modes, so, no this is not pointless.
This is actual something I always wanted to implement !
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
>
>> {
>> drmModeConnector *connector;
>> drmModeModeInfo *mode;
>> int i;
>> - float mode_vrefresh;
>>
>> connector = get_connector_by_id(dev, con_id);
>> if (!connector || !connector->count_modes)
>> @@ -837,16 +842,19 @@ connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
>> for (i = 0; i < connector->count_modes; i++) {
>> mode = &connector->modes[i];
>> if (!strcmp(mode->name, mode_str)) {
>> - /* If the vertical refresh frequency is not specified then return the
>> - * first mode that match with the name. Else, return the mode that match
>> - * the name and the specified vertical refresh frequency.
>> + /* If the vertical refresh frequency is not specified
>> + * then return the first mode that match with the name
>> + * and update corresponding vrefresh in pipe_arg.
>> + * Else, return the mode that match the name and
>> + * the specified vertical refresh frequency.
>> */
>> - mode_vrefresh = mode->clock * 1000.00
>> - / (mode->htotal * mode->vtotal);
>> - if (vrefresh == 0)
>> + if (*vrefresh == 0) {
>> + *vrefresh = mode_vrefresh(mode);
>> return mode;
>> - else if (fabs(mode_vrefresh - vrefresh) < 0.005)
>> + } else if (fabs(mode_vrefresh(mode)
>> + - *vrefresh) < 0.005) {
>> return mode;
>> + }
>> }
>> }
>>
>> @@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct device *dev, struct pipe_arg *pipe)
>>
>> for (i = 0; i < (int)pipe->num_cons; i++) {
>> mode = connector_find_mode(dev, pipe->con_ids[i],
>> - pipe->mode_str, pipe->vrefresh);
>> + pipe->mode_str, &pipe->vrefresh);
>> if (mode == NULL) {
>> - fprintf(stderr,
>> + if (pipe->vrefresh)
>> + fprintf(stderr,
>> + "failed to find mode "
>> + "\"%s-%.2fHz\" for connector %s\n",
>> + pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
>> + else
>> + fprintf(stderr,
>> "failed to find mode \"%s\" for connector %s\n",
>> pipe->mode_str, pipe->cons[i]);
>> return -EINVAL;
>> --
>> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode
2019-12-02 17:22 ` Neil Armstrong
@ 2019-12-02 17:42 ` Ville Syrjälä
2019-12-03 6:49 ` Devarsh Thakkar
0 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2019-12-02 17:42 UTC (permalink / raw)
To: Neil Armstrong; +Cc: rsk, vcu-team, dshah, Devarsh Thakkar, dri-devel, varunkum
On Mon, Dec 02, 2019 at 06:22:56PM +0100, Neil Armstrong wrote:
> On 02/12/2019 18:12, Ville Syrjälä wrote:
> > On Mon, Dec 02, 2019 at 03:27:51AM -0800, Devarsh Thakkar wrote:
> >> Add function to derive floating value of vertical
> >> refresh rate from drm mode using pixel clock,
> >> horizontal total size and vertical total size.
> >>
> >> Use this function to find suitable mode having vrefresh
> >> value which is matching with user provided vrefresh value.
> >>
> >> If user doesn't provide any vrefresh value in args then
> >> update vertical refresh rate value in pipe args using this
> >> function.
> >>
> >> Also use this function for printing floating vrefresh while
> >> dumping all available modes.
> >>
> >> This will give more accurate picture to user for available modes
> >> differentiated by floating vertical refresh rate and help user
> >> select more appropriate mode using suitable refresh rate value.
> >>
> >> V3: Change name of function used to derive refresh rate.
> >> V2: 1) Don't use inline function for deriving refresh rate from mode.
> >> 2) If requested mode not found, print refresh rate only
> >> if user had provided it in args.
> >>
> >> Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
> >> ---
> >> tests/modetest/modetest.c | 40 +++++++++++++++++++++++++++-------------
> >> 1 file changed, 27 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> >> index b4edfcb..19ce20f 100644
> >> --- a/tests/modetest/modetest.c
> >> +++ b/tests/modetest/modetest.c
> >> @@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val)
> >> return (int64_t)*((int64_t *)&val);
> >> }
> >>
> >> +static float mode_vrefresh(drmModeModeInfo *mode)
> >> +{
> >> + return mode->clock * 1000.00
> >> + / (mode->htotal * mode->vtotal);
> >> +}
> >> +
> >> #define bit_name_fn(res) \
> >> const char * res##_str(int type) { \
> >> unsigned int i; \
> >> @@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev)
> >>
> >> static void dump_mode(drmModeModeInfo *mode)
> >> {
> >> - printf(" %s %d %d %d %d %d %d %d %d %d %d",
> >> + printf(" %s %.2f %d %d %d %d %d %d %d %d %d",
> >> mode->name,
> >> - mode->vrefresh,
> >> + mode_vrefresh(mode),
> >> mode->hdisplay,
> >> mode->hsync_start,
> >> mode->hsync_end,
> >> @@ -823,12 +829,11 @@ struct plane_arg {
> >>
> >> static drmModeModeInfo *
> >> connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
> >> - const float vrefresh)
> >> + float *vrefresh)
> >
> > This change still looks pointless.
>
> Without this, you cannot set 1000/1001 CEA alternate clock modes, so, no this is not pointless.
I'm talking about this specific s/float/float*/ change. That
is pointless. Are you talking about the whole patch or what?
>
> This is actual something I always wanted to implement !
>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
>
> >
> >> {
> >> drmModeConnector *connector;
> >> drmModeModeInfo *mode;
> >> int i;
> >> - float mode_vrefresh;
> >>
> >> connector = get_connector_by_id(dev, con_id);
> >> if (!connector || !connector->count_modes)
> >> @@ -837,16 +842,19 @@ connector_find_mode(struct device *dev, uint32_t con_id, const char *mode_str,
> >> for (i = 0; i < connector->count_modes; i++) {
> >> mode = &connector->modes[i];
> >> if (!strcmp(mode->name, mode_str)) {
> >> - /* If the vertical refresh frequency is not specified then return the
> >> - * first mode that match with the name. Else, return the mode that match
> >> - * the name and the specified vertical refresh frequency.
> >> + /* If the vertical refresh frequency is not specified
> >> + * then return the first mode that match with the name
> >> + * and update corresponding vrefresh in pipe_arg.
> >> + * Else, return the mode that match the name and
> >> + * the specified vertical refresh frequency.
> >> */
> >> - mode_vrefresh = mode->clock * 1000.00
> >> - / (mode->htotal * mode->vtotal);
> >> - if (vrefresh == 0)
> >> + if (*vrefresh == 0) {
> >> + *vrefresh = mode_vrefresh(mode);
> >> return mode;
> >> - else if (fabs(mode_vrefresh - vrefresh) < 0.005)
> >> + } else if (fabs(mode_vrefresh(mode)
> >> + - *vrefresh) < 0.005) {
> >> return mode;
> >> + }
> >> }
> >> }
> >>
> >> @@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct device *dev, struct pipe_arg *pipe)
> >>
> >> for (i = 0; i < (int)pipe->num_cons; i++) {
> >> mode = connector_find_mode(dev, pipe->con_ids[i],
> >> - pipe->mode_str, pipe->vrefresh);
> >> + pipe->mode_str, &pipe->vrefresh);
> >> if (mode == NULL) {
> >> - fprintf(stderr,
> >> + if (pipe->vrefresh)
> >> + fprintf(stderr,
> >> + "failed to find mode "
> >> + "\"%s-%.2fHz\" for connector %s\n",
> >> + pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
> >> + else
> >> + fprintf(stderr,
> >> "failed to find mode \"%s\" for connector %s\n",
> >> pipe->mode_str, pipe->cons[i]);
> >> return -EINVAL;
> >> --
> >> 2.7.4
> >
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode
2019-12-02 17:42 ` Ville Syrjälä
@ 2019-12-03 6:49 ` Devarsh Thakkar
2019-12-03 13:01 ` Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Devarsh Thakkar @ 2019-12-03 6:49 UTC (permalink / raw)
To: Ville Syrjälä, Neil Armstrong
Cc: Ranganathan Sk, Varunkumar Allagadapa, vcu-team, dri-devel,
Dhaval Rajeshbhai Shah
Thanks for the review Ville and Neil, response inline.
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: 02 December 2019 09:43
> To: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; Ranganathan Sk
> <rsk@xilinx.com>; vcu-team <vcu-team@xilinx.com>; Dhaval Rajeshbhai Shah
> <dshah@xilinx.com>; dri-devel@lists.freedesktop.org; Varunkumar
> Allagadapa <VARUNKUM@xilinx.com>
> Subject: Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping
> mode
>
> EXTERNAL EMAIL
>
> On Mon, Dec 02, 2019 at 06:22:56PM +0100, Neil Armstrong wrote:
> > On 02/12/2019 18:12, Ville Syrjälä wrote:
> > > On Mon, Dec 02, 2019 at 03:27:51AM -0800, Devarsh Thakkar wrote:
> > >> Add function to derive floating value of vertical refresh rate from
> > >> drm mode using pixel clock, horizontal total size and vertical
> > >> total size.
> > >>
> > >> Use this function to find suitable mode having vrefresh value which
> > >> is matching with user provided vrefresh value.
> > >>
> > >> If user doesn't provide any vrefresh value in args then update
> > >> vertical refresh rate value in pipe args using this function.
> > >>
> > >> Also use this function for printing floating vrefresh while dumping
> > >> all available modes.
> > >>
> > >> This will give more accurate picture to user for available modes
> > >> differentiated by floating vertical refresh rate and help user
> > >> select more appropriate mode using suitable refresh rate value.
> > >>
> > >> V3: Change name of function used to derive refresh rate.
> > >> V2: 1) Don't use inline function for deriving refresh rate from mode.
> > >> 2) If requested mode not found, print refresh rate only
> > >> if user had provided it in args.
> > >>
> > >> Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
> > >> ---
> > >> tests/modetest/modetest.c | 40
> > >> +++++++++++++++++++++++++++-------------
> > >> 1 file changed, 27 insertions(+), 13 deletions(-)
> > >>
> > >> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > >> index b4edfcb..19ce20f 100644
> > >> --- a/tests/modetest/modetest.c
> > >> +++ b/tests/modetest/modetest.c
> > >> @@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val)
> > >> return (int64_t)*((int64_t *)&val); }
> > >>
> > >> +static float mode_vrefresh(drmModeModeInfo *mode) {
> > >> + return mode->clock * 1000.00
> > >> + / (mode->htotal * mode->vtotal); }
> > >> +
> > >> #define bit_name_fn(res) \
> > >> const char * res##_str(int type) { \
> > >> unsigned int i; \
> > >> @@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev)
> > >>
> > >> static void dump_mode(drmModeModeInfo *mode) {
> > >> - printf(" %s %d %d %d %d %d %d %d %d %d %d",
> > >> + printf(" %s %.2f %d %d %d %d %d %d %d %d %d",
> > >> mode->name,
> > >> - mode->vrefresh,
> > >> + mode_vrefresh(mode),
> > >> mode->hdisplay,
> > >> mode->hsync_start,
> > >> mode->hsync_end,
> > >> @@ -823,12 +829,11 @@ struct plane_arg {
> > >>
> > >> static drmModeModeInfo *
> > >> connector_find_mode(struct device *dev, uint32_t con_id, const
> > >> char *mode_str,
> > >> - const float vrefresh)
> > >> + float *vrefresh)
> > >
> > > This change still looks pointless.
> >
The only small advantage it offers is that it backs up the vrefresh from mode in pipe_args in connector_find_mode and I don't have to call mode_vrefresh again while printing below :
printf("setting mode %s-%.2fHz on connectors ",
pipe->mode_str, pipe->vrefresh);
If this is not preferable then it's not mandatory either, then instead of doing this I can call mode_vrefresh again while printing the mode as below :
printf("setting mode %s-%.2fHz on connectors ",
pipe->mode_str, mode_vrefresh(pipe->mode))
Kindly let me know your opinion.
> > Without this, you cannot set 1000/1001 CEA alternate clock modes, so, no
> this is not pointless.
>
> I'm talking about this specific s/float/float*/ change. That is pointless. Are you
> talking about the whole patch or what?
>
> >
> > This is actual something I always wanted to implement !
> >
> > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> >
Thanks for the review Neil.
> > >
> > >> {
> > >> drmModeConnector *connector;
> > >> drmModeModeInfo *mode;
> > >> int i;
> > >> - float mode_vrefresh;
> > >>
> > >> connector = get_connector_by_id(dev, con_id);
> > >> if (!connector || !connector->count_modes) @@ -837,16 +842,19
> @@
> > >> connector_find_mode(struct device *dev, uint32_t con_id, const char
> *mode_str,
> > >> for (i = 0; i < connector->count_modes; i++) {
> > >> mode = &connector->modes[i];
> > >> if (!strcmp(mode->name, mode_str)) {
> > >> - /* If the vertical refresh frequency is not specified then
> return the
> > >> - * first mode that match with the name. Else, return the mode
> that match
> > >> - * the name and the specified vertical refresh frequency.
> > >> + /* If the vertical refresh frequency is not specified
> > >> + * then return the first mode that match with the name
> > >> + * and update corresponding vrefresh in pipe_arg.
> > >> + * Else, return the mode that match the name and
> > >> + * the specified vertical refresh frequency.
> > >> */
> > >> - mode_vrefresh = mode->clock * 1000.00
> > >> - / (mode->htotal * mode->vtotal);
> > >> - if (vrefresh == 0)
> > >> + if (*vrefresh == 0) {
> > >> + *vrefresh = mode_vrefresh(mode);
> > >> return mode;
> > >> - else if (fabs(mode_vrefresh - vrefresh) < 0.005)
> > >> + } else if (fabs(mode_vrefresh(mode)
> > >> + - *vrefresh) < 0.005) {
> > >> return mode;
> > >> + }
> > >> }
> > >> }
> > >>
> > >> @@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct
> > >> device *dev, struct pipe_arg *pipe)
> > >>
> > >> for (i = 0; i < (int)pipe->num_cons; i++) {
> > >> mode = connector_find_mode(dev, pipe->con_ids[i],
> > >> - pipe->mode_str, pipe->vrefresh);
> > >> + pipe->mode_str,
> > >> + &pipe->vrefresh);
> > >> if (mode == NULL) {
> > >> - fprintf(stderr,
> > >> + if (pipe->vrefresh)
> > >> + fprintf(stderr,
> > >> + "failed to find mode "
> > >> + "\"%s-%.2fHz\" for connector %s\n",
> > >> + pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
> > >> + else
> > >> + fprintf(stderr,
> > >> "failed to find mode \"%s\" for connector %s\n",
> > >> pipe->mode_str, pipe->cons[i]);
> > >> return -EINVAL;
> > >> --
> > >> 2.7.4
> > >
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode
2019-12-03 6:49 ` Devarsh Thakkar
@ 2019-12-03 13:01 ` Ville Syrjälä
0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2019-12-03 13:01 UTC (permalink / raw)
To: Devarsh Thakkar
Cc: Ranganathan Sk, vcu-team, Dhaval Rajeshbhai Shah, Neil Armstrong,
dri-devel, Varunkumar Allagadapa
On Tue, Dec 03, 2019 at 06:49:31AM +0000, Devarsh Thakkar wrote:
> Thanks for the review Ville and Neil, response inline.
>
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: 02 December 2019 09:43
> > To: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Devarsh Thakkar <DEVARSHT@xilinx.com>; Ranganathan Sk
> > <rsk@xilinx.com>; vcu-team <vcu-team@xilinx.com>; Dhaval Rajeshbhai Shah
> > <dshah@xilinx.com>; dri-devel@lists.freedesktop.org; Varunkumar
> > Allagadapa <VARUNKUM@xilinx.com>
> > Subject: Re: [PATCH libdrm v3] modetest: Use floating vrefresh while dumping
> > mode
> >
> > EXTERNAL EMAIL
> >
> > On Mon, Dec 02, 2019 at 06:22:56PM +0100, Neil Armstrong wrote:
> > > On 02/12/2019 18:12, Ville Syrjälä wrote:
> > > > On Mon, Dec 02, 2019 at 03:27:51AM -0800, Devarsh Thakkar wrote:
> > > >> Add function to derive floating value of vertical refresh rate from
> > > >> drm mode using pixel clock, horizontal total size and vertical
> > > >> total size.
> > > >>
> > > >> Use this function to find suitable mode having vrefresh value which
> > > >> is matching with user provided vrefresh value.
> > > >>
> > > >> If user doesn't provide any vrefresh value in args then update
> > > >> vertical refresh rate value in pipe args using this function.
> > > >>
> > > >> Also use this function for printing floating vrefresh while dumping
> > > >> all available modes.
> > > >>
> > > >> This will give more accurate picture to user for available modes
> > > >> differentiated by floating vertical refresh rate and help user
> > > >> select more appropriate mode using suitable refresh rate value.
> > > >>
> > > >> V3: Change name of function used to derive refresh rate.
> > > >> V2: 1) Don't use inline function for deriving refresh rate from mode.
> > > >> 2) If requested mode not found, print refresh rate only
> > > >> if user had provided it in args.
> > > >>
> > > >> Signed-off-by: Devarsh Thakkar <devarsh.thakkar@xilinx.com>
> > > >> ---
> > > >> tests/modetest/modetest.c | 40
> > > >> +++++++++++++++++++++++++++-------------
> > > >> 1 file changed, 27 insertions(+), 13 deletions(-)
> > > >>
> > > >> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > >> index b4edfcb..19ce20f 100644
> > > >> --- a/tests/modetest/modetest.c
> > > >> +++ b/tests/modetest/modetest.c
> > > >> @@ -133,6 +133,12 @@ static inline int64_t U642I64(uint64_t val)
> > > >> return (int64_t)*((int64_t *)&val); }
> > > >>
> > > >> +static float mode_vrefresh(drmModeModeInfo *mode) {
> > > >> + return mode->clock * 1000.00
> > > >> + / (mode->htotal * mode->vtotal); }
> > > >> +
> > > >> #define bit_name_fn(res) \
> > > >> const char * res##_str(int type) { \
> > > >> unsigned int i; \
> > > >> @@ -210,9 +216,9 @@ static void dump_encoders(struct device *dev)
> > > >>
> > > >> static void dump_mode(drmModeModeInfo *mode) {
> > > >> - printf(" %s %d %d %d %d %d %d %d %d %d %d",
> > > >> + printf(" %s %.2f %d %d %d %d %d %d %d %d %d",
> > > >> mode->name,
> > > >> - mode->vrefresh,
> > > >> + mode_vrefresh(mode),
> > > >> mode->hdisplay,
> > > >> mode->hsync_start,
> > > >> mode->hsync_end,
> > > >> @@ -823,12 +829,11 @@ struct plane_arg {
> > > >>
> > > >> static drmModeModeInfo *
> > > >> connector_find_mode(struct device *dev, uint32_t con_id, const
> > > >> char *mode_str,
> > > >> - const float vrefresh)
> > > >> + float *vrefresh)
> > > >
> > > > This change still looks pointless.
> > >
>
> The only small advantage it offers is that it backs up the vrefresh from mode in pipe_args in connector_find_mode and I don't have to call mode_vrefresh again while printing below :
> printf("setting mode %s-%.2fHz on connectors ",
> pipe->mode_str, pipe->vrefresh);
>
> If this is not preferable then it's not mandatory either, then instead of doing this I can call mode_vrefresh again while printing the mode as below :
> printf("setting mode %s-%.2fHz on connectors ",
> pipe->mode_str, mode_vrefresh(pipe->mode))
I would change that to to use mode_vrefres(), and also IMO better to
replace the pipe->mode_str with mode->name as well. That way the code
doesn't have to care at all how the mode was chosen.
>
> Kindly let me know your opinion.
>
> > > Without this, you cannot set 1000/1001 CEA alternate clock modes, so, no
> > this is not pointless.
> >
> > I'm talking about this specific s/float/float*/ change. That is pointless. Are you
> > talking about the whole patch or what?
>
> >
> > >
> > > This is actual something I always wanted to implement !
> > >
> > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> > >
>
> Thanks for the review Neil.
>
> > > >
> > > >> {
> > > >> drmModeConnector *connector;
> > > >> drmModeModeInfo *mode;
> > > >> int i;
> > > >> - float mode_vrefresh;
> > > >>
> > > >> connector = get_connector_by_id(dev, con_id);
> > > >> if (!connector || !connector->count_modes) @@ -837,16 +842,19
> > @@
> > > >> connector_find_mode(struct device *dev, uint32_t con_id, const char
> > *mode_str,
> > > >> for (i = 0; i < connector->count_modes; i++) {
> > > >> mode = &connector->modes[i];
> > > >> if (!strcmp(mode->name, mode_str)) {
> > > >> - /* If the vertical refresh frequency is not specified then
> > return the
> > > >> - * first mode that match with the name. Else, return the mode
> > that match
> > > >> - * the name and the specified vertical refresh frequency.
> > > >> + /* If the vertical refresh frequency is not specified
> > > >> + * then return the first mode that match with the name
> > > >> + * and update corresponding vrefresh in pipe_arg.
> > > >> + * Else, return the mode that match the name and
> > > >> + * the specified vertical refresh frequency.
> > > >> */
> > > >> - mode_vrefresh = mode->clock * 1000.00
> > > >> - / (mode->htotal * mode->vtotal);
> > > >> - if (vrefresh == 0)
> > > >> + if (*vrefresh == 0) {
> > > >> + *vrefresh = mode_vrefresh(mode);
> > > >> return mode;
> > > >> - else if (fabs(mode_vrefresh - vrefresh) < 0.005)
> > > >> + } else if (fabs(mode_vrefresh(mode)
> > > >> + - *vrefresh) < 0.005) {
> > > >> return mode;
> > > >> + }
> > > >> }
> > > >> }
> > > >>
> > > >> @@ -909,9 +917,15 @@ static int pipe_find_crtc_and_mode(struct
> > > >> device *dev, struct pipe_arg *pipe)
> > > >>
> > > >> for (i = 0; i < (int)pipe->num_cons; i++) {
> > > >> mode = connector_find_mode(dev, pipe->con_ids[i],
> > > >> - pipe->mode_str, pipe->vrefresh);
> > > >> + pipe->mode_str,
> > > >> + &pipe->vrefresh);
> > > >> if (mode == NULL) {
> > > >> - fprintf(stderr,
> > > >> + if (pipe->vrefresh)
> > > >> + fprintf(stderr,
> > > >> + "failed to find mode "
> > > >> + "\"%s-%.2fHz\" for connector %s\n",
> > > >> + pipe->mode_str, pipe->vrefresh, pipe->cons[i]);
> > > >> + else
> > > >> + fprintf(stderr,
> > > >> "failed to find mode \"%s\" for connector %s\n",
> > > >> pipe->mode_str, pipe->cons[i]);
> > > >> return -EINVAL;
> > > >> --
> > > >> 2.7.4
> > > >
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-03 13:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 11:27 [PATCH libdrm v3] modetest: Use floating vrefresh while dumping mode Devarsh Thakkar
2019-12-02 17:12 ` Ville Syrjälä
2019-12-02 17:22 ` Neil Armstrong
2019-12-02 17:42 ` Ville Syrjälä
2019-12-03 6:49 ` Devarsh Thakkar
2019-12-03 13:01 ` Ville Syrjälä
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).