Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-24 Thread Ville Syrjälä 
On Fri, Nov 24, 2017 at 02:26:09PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/17/2017 6:19 PM, Ville Syrjälä wrote:
> > On Fri, Nov 17, 2017 at 05:50:11PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/17/2017 5:05 PM, Ville Syrjälä wrote:
> >>> On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:
>  Regards
> 
>  Shashank
> 
> 
>  On 11/16/2017 9:53 PM, Ville Syrjälä wrote:
> > On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
> >>> From: Ville Syrjälä 
> >>>
> >>> commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
> >>> cause us to not send out any VICs in the AVI infoframes. That commit
> >>> was since reverted, but if and when we add aspect ratio handing back
> >>> we need to be more careful.
> >>>
> >>> Let's handle this by considering the aspect ratio as a requirement
> >>> for cea mode matching only if the passed in mode actually has a
> >>> non-zero aspect ratio field. This will keep userspace that doesn't
> >>> provide an aspect ratio working as before by matching it to the
> >>> first otherwise equal cea mode. And once userspace starts to
> >>> provide the aspect ratio it will be considerd a hard requirement
> >>> for the match.
> >>>
> >>> Also change the hdmi mode matching to use drm_mode_match() for
> >>> consistency, but we don't match on aspect ratio there since the
> >>> spec doesn't list a specific aspect ratio for those modes.
> >>>
> >>> Cc: Shashank Sharma 
> >>> Cc: "Lin, Jia" 
> >>> Cc: Akashdeep Sharma 
> >>> Cc: Jim Bride 
> >>> Cc: Jose Abreu 
> >>> Cc: Daniel Vetter 
> >>> Cc: Emil Velikov 
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>  drivers/gpu/drm/drm_edid.c | 18 ++
> >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>> index 7220b8f9a7e8..00aa98f3e55d 100644
> >>> --- a/drivers/gpu/drm/drm_edid.c
> >>> +++ b/drivers/gpu/drm/drm_edid.c
> >>> @@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
> >>> drm_display_mode *mode)
> >>>  static u8 drm_match_cea_mode_clock_tolerance(const struct 
> >>> drm_display_mode *to_match,
> >>>unsigned int 
> >>> clock_tolerance)
> >>>  {
> >>> + unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
> >>> DRM_MODE_MATCH_FLAGS;
> >>>   u8 vic;
> >>>  
> >>>   if (!to_match->clock)
> >>>   return 0;
> >>>  
> >>> + if (to_match->picture_aspect_ratio)
> >>> + match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> >> This doesn't look right. This means we are expecting a CEA mode without
> >> a pic aspect ratio field,
> >> which is invalid.
> > No, it's perfectly valid. It's what we currently get from userspace.
>  Yep, but that's due to missing Aspect ratio handling in the DRM layer.
>  If that's fixed, as per the list of CEA modes,
>  each CEA VIC contains an aspect ratio, which is a part of its unique
>  identity.
> 
>  I guess once we have the aspect ratio handling in DRM layer, it
>  would/should look like this:
>  - EDID gives you all supported modes, including CEA modes with Aspect 
>  ratio
>  - Userspcae gets the mode information, with aspect ratio (for CEA modes)
>  If ( Userspace picks one of the CEA modes)
> - sends a modeset
> - we find a matching CEA VIC, found one from modedb
> - we load this VIC = nonzero information in AVI IF VIC field,
>  else
> - sends a modeset
> - we could not find a matching CEA VIC, as aspect ratio is 0
> - we make VIC field in AVI IF as 0
> >>> No. That would break current userspace.
> >> I guess I forgot to make it clear, that userspace will set the cap, only
> >> then we will provide aspect ratio information.
> >> So this should not break userspace, isn't it ?
>  This is important, as HDMI compliance test 7-27 inspects if the VIC
>  field in the AVI IF is accurate.
> >>> Complicance is secondary to not breaking things that work. Also I find
> >>> it hard to see what purpose there is in having a complicance test that
> >>> sets a CEA modes w/o aspect ratio and then expects the infoframe to have
> >>> VIC 0.
> >> Again, typically this is how these analyzers force

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-24 Thread Sharma, Shashank 

Regards

Shashank


On 11/17/2017 6:19 PM, Ville Syrjälä wrote:

On Fri, Nov 17, 2017 at 05:50:11PM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 11/17/2017 5:05 PM, Ville Syrjälä wrote:

On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 11/16/2017 9:53 PM, Ville Syrjälä wrote:

On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 11/13/2017 10:34 PM, Ville Syrjala wrote:

From: Ville Syrjälä 

commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
cause us to not send out any VICs in the AVI infoframes. That commit
was since reverted, but if and when we add aspect ratio handing back
we need to be more careful.

Let's handle this by considering the aspect ratio as a requirement
for cea mode matching only if the passed in mode actually has a
non-zero aspect ratio field. This will keep userspace that doesn't
provide an aspect ratio working as before by matching it to the
first otherwise equal cea mode. And once userspace starts to
provide the aspect ratio it will be considerd a hard requirement
for the match.

Also change the hdmi mode matching to use drm_mode_match() for
consistency, but we don't match on aspect ratio there since the
spec doesn't list a specific aspect ratio for those modes.

Cc: Shashank Sharma 
Cc: "Lin, Jia" 
Cc: Akashdeep Sharma 
Cc: Jim Bride 
Cc: Jose Abreu 
Cc: Daniel Vetter 
Cc: Emil Velikov 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7220b8f9a7e8..00aa98f3e55d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
drm_display_mode *mode)
 static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
 unsigned int clock_tolerance)
 {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
 
 	if (!to_match->clock)

return 0;
 
+	if (to_match->picture_aspect_ratio)

+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;

This doesn't look right. This means we are expecting a CEA mode without
a pic aspect ratio field,
which is invalid.

No, it's perfectly valid. It's what we currently get from userspace.

Yep, but that's due to missing Aspect ratio handling in the DRM layer.
If that's fixed, as per the list of CEA modes,
each CEA VIC contains an aspect ratio, which is a part of its unique
identity.

I guess once we have the aspect ratio handling in DRM layer, it
would/should look like this:
- EDID gives you all supported modes, including CEA modes with Aspect ratio
- Userspcae gets the mode information, with aspect ratio (for CEA modes)
If ( Userspace picks one of the CEA modes)
   - sends a modeset
   - we find a matching CEA VIC, found one from modedb
   - we load this VIC = nonzero information in AVI IF VIC field,
else
   - sends a modeset
   - we could not find a matching CEA VIC, as aspect ratio is 0
   - we make VIC field in AVI IF as 0a

No. That would break current userspace.

I guess I forgot to make it clear, that userspace will set the cap, only
then we will provide aspect ratio information.
So this should not break userspace, isn't it ?

This is important, as HDMI compliance test 7-27 inspects if the VIC
field in the AVI IF is accurate.

Complicance is secondary to not breaking things that work. Also I find
it hard to see what purpose there is in having a complicance test that
sets a CEA modes w/o aspect ratio and then expects the infoframe to have
VIC 0.

Again, typically this is how these analyzers force modeset:
- They send EDID with only one mode, which is the test mode, with aspect
ratio.
- They expect that VIC to be present in AVI IF

- Shashank

Ankit is going to publish the aspect ratio patch
series again, with proper DRM cap and flags check. Would it be
ok if we have a look that handling first ?

This patch will be needed by that work. Otherwise we're going to stop
sending a VIC for CEA modes with current userspace.

I guess we should force userspaces to start bothering about aspect ratio
field, right now we
are doing this for Wayland based compositors, may be we should extend it
to X based too.

Yes, I've been saying that someone should look into extending the randr
protocol with the necessary bits. But that still doesn't allow us to
change the current behaviour as old userspace would anyway linger around
for a long time.

I think cap will cove this part

The cap is irrelevant to this discussion. It

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-17 Thread Ville Syrjälä 
On Fri, Nov 17, 2017 at 05:50:11PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/17/2017 5:05 PM, Ville Syrjälä wrote:
> > On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/16/2017 9:53 PM, Ville Syrjälä wrote:
> >>> On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:
>  Regards
> 
>  Shashank
> 
> 
>  On 11/13/2017 10:34 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> >
> > commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
> > cause us to not send out any VICs in the AVI infoframes. That commit
> > was since reverted, but if and when we add aspect ratio handing back
> > we need to be more careful.
> >
> > Let's handle this by considering the aspect ratio as a requirement
> > for cea mode matching only if the passed in mode actually has a
> > non-zero aspect ratio field. This will keep userspace that doesn't
> > provide an aspect ratio working as before by matching it to the
> > first otherwise equal cea mode. And once userspace starts to
> > provide the aspect ratio it will be considerd a hard requirement
> > for the match.
> >
> > Also change the hdmi mode matching to use drm_mode_match() for
> > consistency, but we don't match on aspect ratio there since the
> > spec doesn't list a specific aspect ratio for those modes.
> >
> > Cc: Shashank Sharma 
> > Cc: "Lin, Jia" 
> > Cc: Akashdeep Sharma 
> > Cc: Jim Bride 
> > Cc: Jose Abreu 
> > Cc: Daniel Vetter 
> > Cc: Emil Velikov 
> > Signed-off-by: Ville Syrjälä 
> > ---
> > drivers/gpu/drm/drm_edid.c | 18 ++
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 7220b8f9a7e8..00aa98f3e55d 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
> > drm_display_mode *mode)
> > static u8 drm_match_cea_mode_clock_tolerance(const struct 
> > drm_display_mode *to_match,
> >  unsigned int 
> > clock_tolerance)
> > {
> > +   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
> > DRM_MODE_MATCH_FLAGS;
> > u8 vic;
> > 
> > if (!to_match->clock)
> > return 0;
> > 
> > +   if (to_match->picture_aspect_ratio)
> > +   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
>  This doesn't look right. This means we are expecting a CEA mode without
>  a pic aspect ratio field,
>  which is invalid.
> >>> No, it's perfectly valid. It's what we currently get from userspace.
> >> Yep, but that's due to missing Aspect ratio handling in the DRM layer.
> >> If that's fixed, as per the list of CEA modes,
> >> each CEA VIC contains an aspect ratio, which is a part of its unique
> >> identity.
> >>
> >> I guess once we have the aspect ratio handling in DRM layer, it
> >> would/should look like this:
> >> - EDID gives you all supported modes, including CEA modes with Aspect ratio
> >> - Userspcae gets the mode information, with aspect ratio (for CEA modes)
> >> If ( Userspace picks one of the CEA modes)
> >>   - sends a modeset
> >>   - we find a matching CEA VIC, found one from modedb
> >>   - we load this VIC = nonzero information in AVI IF VIC field,
> >> else
> >>   - sends a modeset
> >>   - we could not find a matching CEA VIC, as aspect ratio is 0
> >>   - we make VIC field in AVI IF as 0a
> > No. That would break current userspace.
> I guess I forgot to make it clear, that userspace will set the cap, only 
> then we will provide aspect ratio information.
> So this should not break userspace, isn't it ?
> >> This is important, as HDMI compliance test 7-27 inspects if the VIC
> >> field in the AVI IF is accurate.
> > Complicance is secondary to not breaking things that work. Also I find
> > it hard to see what purpose there is in having a complicance test that
> > sets a CEA modes w/o aspect ratio and then expects the infoframe to have
> > VIC 0.
> Again, typically this is how these analyzers force modeset:
> - They send EDID with only one mode, which is the test mode, with aspect 
> ratio.
> - They expect that VIC to be present in AVI IF
> >> - Shashank
>  Ankit is going to publish the aspect ratio patch
>  series again, with proper DRM cap and flags check. Would it be
>  ok if we have a look that handling first ?
> >>> This patch will be

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-17 Thread Sharma, Shashank 

Regards

Shashank


On 11/17/2017 5:05 PM, Ville Syrjälä wrote:

On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 11/16/2017 9:53 PM, Ville Syrjälä wrote:

On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 11/13/2017 10:34 PM, Ville Syrjala wrote:

From: Ville Syrjälä 

commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
cause us to not send out any VICs in the AVI infoframes. That commit
was since reverted, but if and when we add aspect ratio handing back
we need to be more careful.

Let's handle this by considering the aspect ratio as a requirement
for cea mode matching only if the passed in mode actually has a
non-zero aspect ratio field. This will keep userspace that doesn't
provide an aspect ratio working as before by matching it to the
first otherwise equal cea mode. And once userspace starts to
provide the aspect ratio it will be considerd a hard requirement
for the match.

Also change the hdmi mode matching to use drm_mode_match() for
consistency, but we don't match on aspect ratio there since the
spec doesn't list a specific aspect ratio for those modes.

Cc: Shashank Sharma 
Cc: "Lin, Jia" 
Cc: Akashdeep Sharma 
Cc: Jim Bride 
Cc: Jose Abreu 
Cc: Daniel Vetter 
Cc: Emil Velikov 
Signed-off-by: Ville Syrjälä 
---
drivers/gpu/drm/drm_edid.c | 18 ++
1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7220b8f9a7e8..00aa98f3e55d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
drm_display_mode *mode)
static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
 unsigned int clock_tolerance)
{
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;

	if (!to_match->clock)

return 0;

+	if (to_match->picture_aspect_ratio)

+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;

This doesn't look right. This means we are expecting a CEA mode without
a pic aspect ratio field,
which is invalid.

No, it's perfectly valid. It's what we currently get from userspace.

Yep, but that's due to missing Aspect ratio handling in the DRM layer.
If that's fixed, as per the list of CEA modes,
each CEA VIC contains an aspect ratio, which is a part of its unique
identity.

I guess once we have the aspect ratio handling in DRM layer, it
would/should look like this:
- EDID gives you all supported modes, including CEA modes with Aspect ratio
- Userspcae gets the mode information, with aspect ratio (for CEA modes)
If ( Userspace picks one of the CEA modes)
  - sends a modeset
  - we find a matching CEA VIC, found one from modedb
  - we load this VIC = nonzero information in AVI IF VIC field,
else
  - sends a modeset
  - we could not find a matching CEA VIC, as aspect ratio is 0
  - we make VIC field in AVI IF as 0a

No. That would break current userspace.
I guess I forgot to make it clear, that userspace will set the cap, only 
then we will provide aspect ratio information.

So this should not break userspace, isn't it ?

This is important, as HDMI compliance test 7-27 inspects if the VIC
field in the AVI IF is accurate.

Complicance is secondary to not breaking things that work. Also I find
it hard to see what purpose there is in having a complicance test that
sets a CEA modes w/o aspect ratio and then expects the infoframe to have
VIC 0.

Again, typically this is how these analyzers force modeset:
- They send EDID with only one mode, which is the test mode, with aspect 
ratio.

- They expect that VIC to be present in AVI IF

- Shashank

Ankit is going to publish the aspect ratio patch
series again, with proper DRM cap and flags check. Would it be
ok if we have a look that handling first ?

This patch will be needed by that work. Otherwise we're going to stop
sending a VIC for CEA modes with current userspace.

I guess we should force userspaces to start bothering about aspect ratio
field, right now we
are doing this for Wayland based compositors, may be we should extend it
to X based too.

Yes, I've been saying that someone should look into extending the randr
protocol with the necessary bits. But that still doesn't allow us to
change the current behaviour as old userspace would anyway linger around
for a long time.

I think cap will cove this part

- Shashank

- Shashank

+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct drm_display_mode cea_mode = edid_cea_modes[vic];

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-17 Thread Ville Syrjälä 
On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/16/2017 9:53 PM, Ville Syrjälä wrote:
> > On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
> >>> From: Ville Syrjälä 
> >>>
> >>> commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
> >>> cause us to not send out any VICs in the AVI infoframes. That commit
> >>> was since reverted, but if and when we add aspect ratio handing back
> >>> we need to be more careful.
> >>>
> >>> Let's handle this by considering the aspect ratio as a requirement
> >>> for cea mode matching only if the passed in mode actually has a
> >>> non-zero aspect ratio field. This will keep userspace that doesn't
> >>> provide an aspect ratio working as before by matching it to the
> >>> first otherwise equal cea mode. And once userspace starts to
> >>> provide the aspect ratio it will be considerd a hard requirement
> >>> for the match.
> >>>
> >>> Also change the hdmi mode matching to use drm_mode_match() for
> >>> consistency, but we don't match on aspect ratio there since the
> >>> spec doesn't list a specific aspect ratio for those modes.
> >>>
> >>> Cc: Shashank Sharma 
> >>> Cc: "Lin, Jia" 
> >>> Cc: Akashdeep Sharma 
> >>> Cc: Jim Bride 
> >>> Cc: Jose Abreu 
> >>> Cc: Daniel Vetter 
> >>> Cc: Emil Velikov 
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>drivers/gpu/drm/drm_edid.c | 18 ++
> >>>1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>> index 7220b8f9a7e8..00aa98f3e55d 100644
> >>> --- a/drivers/gpu/drm/drm_edid.c
> >>> +++ b/drivers/gpu/drm/drm_edid.c
> >>> @@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
> >>> drm_display_mode *mode)
> >>>static u8 drm_match_cea_mode_clock_tolerance(const struct 
> >>> drm_display_mode *to_match,
> >>>unsigned int 
> >>> clock_tolerance)
> >>>{
> >>> + unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
> >>> DRM_MODE_MATCH_FLAGS;
> >>>   u8 vic;
> >>>
> >>>   if (!to_match->clock)
> >>>   return 0;
> >>>
> >>> + if (to_match->picture_aspect_ratio)
> >>> + match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> >> This doesn't look right. This means we are expecting a CEA mode without
> >> a pic aspect ratio field,
> >> which is invalid.
> > No, it's perfectly valid. It's what we currently get from userspace.
> Yep, but that's due to missing Aspect ratio handling in the DRM layer. 
> If that's fixed, as per the list of CEA modes,
> each CEA VIC contains an aspect ratio, which is a part of its unique 
> identity.
> 
> I guess once we have the aspect ratio handling in DRM layer, it 
> would/should look like this:
> - EDID gives you all supported modes, including CEA modes with Aspect ratio
> - Userspcae gets the mode information, with aspect ratio (for CEA modes)
> If ( Userspace picks one of the CEA modes)
>  - sends a modeset
>  - we find a matching CEA VIC, found one from modedb
>  - we load this VIC = nonzero information in AVI IF VIC field,
> else
>  - sends a modeset
>  - we could not find a matching CEA VIC, as aspect ratio is 0
>  - we make VIC field in AVI IF as 0a

No. That would break current userspace.

> 
> This is important, as HDMI compliance test 7-27 inspects if the VIC 
> field in the AVI IF is accurate.

Complicance is secondary to not breaking things that work. Also I find
it hard to see what purpose there is in having a complicance test that
sets a CEA modes w/o aspect ratio and then expects the infoframe to have
VIC 0.

> 
> - Shashank
> >> Ankit is going to publish the aspect ratio patch
> >> series again, with proper DRM cap and flags check. Would it be
> >> ok if we have a look that handling first ?
> > This patch will be needed by that work. Otherwise we're going to stop
> > sending a VIC for CEA modes with current userspace.
> I guess we should force userspaces to start bothering about aspect ratio 
> field, right now we
> are doing this for Wayland based compositors, may be we should extend it 
> to X based too.

Yes, I've been saying that someone should look into extending the randr
protocol with the necessary bits. But that still doesn't allow us to
change the current behaviour as old userspace would anyway linger around
for a long time.

> 
> - Shashank
> >
> >>> +
> >>>   for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> >>>   struct drm_display_mode cea_mode = edid_cea_modes[vic];
> >>>   unsigned int

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-16 Thread Sharma, Shashank 

Regards

Shashank


On 11/16/2017 9:53 PM, Ville Syrjälä wrote:

On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 11/13/2017 10:34 PM, Ville Syrjala wrote:

From: Ville Syrjälä 

commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
cause us to not send out any VICs in the AVI infoframes. That commit
was since reverted, but if and when we add aspect ratio handing back
we need to be more careful.

Let's handle this by considering the aspect ratio as a requirement
for cea mode matching only if the passed in mode actually has a
non-zero aspect ratio field. This will keep userspace that doesn't
provide an aspect ratio working as before by matching it to the
first otherwise equal cea mode. And once userspace starts to
provide the aspect ratio it will be considerd a hard requirement
for the match.

Also change the hdmi mode matching to use drm_mode_match() for
consistency, but we don't match on aspect ratio there since the
spec doesn't list a specific aspect ratio for those modes.

Cc: Shashank Sharma 
Cc: "Lin, Jia" 
Cc: Akashdeep Sharma 
Cc: Jim Bride 
Cc: Jose Abreu 
Cc: Daniel Vetter 
Cc: Emil Velikov 
Signed-off-by: Ville Syrjälä 
---
   drivers/gpu/drm/drm_edid.c | 18 ++
   1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7220b8f9a7e8..00aa98f3e55d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
drm_display_mode *mode)
   static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
 unsigned int clock_tolerance)
   {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
   
   	if (!to_match->clock)

return 0;
   
+	if (to_match->picture_aspect_ratio)

+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;

This doesn't look right. This means we are expecting a CEA mode without
a pic aspect ratio field,
which is invalid.

No, it's perfectly valid. It's what we currently get from userspace.
Yep, but that's due to missing Aspect ratio handling in the DRM layer. 
If that's fixed, as per the list of CEA modes,
each CEA VIC contains an aspect ratio, which is a part of its unique 
identity.


I guess once we have the aspect ratio handling in DRM layer, it 
would/should look like this:

- EDID gives you all supported modes, including CEA modes with Aspect ratio
- Userspcae gets the mode information, with aspect ratio (for CEA modes)
If ( Userspace picks one of the CEA modes)
- sends a modeset
- we find a matching CEA VIC, found one from modedb
- we load this VIC = nonzero information in AVI IF VIC field,
else
- sends a modeset
- we could not find a matching CEA VIC, as aspect ratio is 0
- we make VIC field in AVI IF as 0

This is important, as HDMI compliance test 7-27 inspects if the VIC 
field in the AVI IF is accurate.


- Shashank

Ankit is going to publish the aspect ratio patch
series again, with proper DRM cap and flags check. Would it be
ok if we have a look that handling first ?

This patch will be needed by that work. Otherwise we're going to stop
sending a VIC for CEA modes with current userspace.
I guess we should force userspaces to start bothering about aspect ratio 
field, right now we
are doing this for Wayland based compositors, may be we should extend it 
to X based too.


- Shashank



+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct drm_display_mode cea_mode = edid_cea_modes[vic];
unsigned int clock1, clock2;
@@ -2921,7 +2925,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct 
drm_display_mode *to_m
continue;
   
   		do {

-   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
_mode))
+   if (drm_mode_match(to_match, _mode, match_flags))
return vic;
} while (cea_mode_alternate_timings(vic, _mode));
}
@@ -2938,11 +2942,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const 
struct drm_display_mode *to_m
*/
   u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
   {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
   
   	if (!to_match->clock)

return 0;
   
+	if (to_match->picture_aspect_ratio)

+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;

same here, and probably in other CEA match functions.

+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-16 Thread Ville Syrjälä 
On Thu, Nov 16, 2017 at 08:21:44PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> >
> > commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
> > cause us to not send out any VICs in the AVI infoframes. That commit
> > was since reverted, but if and when we add aspect ratio handing back
> > we need to be more careful.
> >
> > Let's handle this by considering the aspect ratio as a requirement
> > for cea mode matching only if the passed in mode actually has a
> > non-zero aspect ratio field. This will keep userspace that doesn't
> > provide an aspect ratio working as before by matching it to the
> > first otherwise equal cea mode. And once userspace starts to
> > provide the aspect ratio it will be considerd a hard requirement
> > for the match.
> >
> > Also change the hdmi mode matching to use drm_mode_match() for
> > consistency, but we don't match on aspect ratio there since the
> > spec doesn't list a specific aspect ratio for those modes.
> >
> > Cc: Shashank Sharma 
> > Cc: "Lin, Jia" 
> > Cc: Akashdeep Sharma 
> > Cc: Jim Bride 
> > Cc: Jose Abreu 
> > Cc: Daniel Vetter 
> > Cc: Emil Velikov 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >   drivers/gpu/drm/drm_edid.c | 18 ++
> >   1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 7220b8f9a7e8..00aa98f3e55d 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
> > drm_display_mode *mode)
> >   static u8 drm_match_cea_mode_clock_tolerance(const struct 
> > drm_display_mode *to_match,
> >  unsigned int clock_tolerance)
> >   {
> > +   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
> > DRM_MODE_MATCH_FLAGS;
> > u8 vic;
> >   
> > if (!to_match->clock)
> > return 0;
> >   
> > +   if (to_match->picture_aspect_ratio)
> > +   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> This doesn't look right. This means we are expecting a CEA mode without 
> a pic aspect ratio field,
> which is invalid.

No, it's perfectly valid. It's what we currently get from userspace.

> Ankit is going to publish the aspect ratio patch 
> series again, with proper DRM cap and flags check. Would it be
> ok if we have a look that handling first ?

This patch will be needed by that work. Otherwise we're going to stop
sending a VIC for CEA modes with current userspace.

> > +
> > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> > struct drm_display_mode cea_mode = edid_cea_modes[vic];
> > unsigned int clock1, clock2;
> > @@ -2921,7 +2925,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const 
> > struct drm_display_mode *to_m
> > continue;
> >   
> > do {
> > -   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
> > _mode))
> > +   if (drm_mode_match(to_match, _mode, match_flags))
> > return vic;
> > } while (cea_mode_alternate_timings(vic, _mode));
> > }
> > @@ -2938,11 +2942,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const 
> > struct drm_display_mode *to_m
> >*/
> >   u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
> >   {
> > +   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
> > DRM_MODE_MATCH_FLAGS;
> > u8 vic;
> >   
> > if (!to_match->clock)
> > return 0;
> >   
> > +   if (to_match->picture_aspect_ratio)
> > +   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> same here, and probably in other CEA match functions.
> > +
> > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
> > struct drm_display_mode cea_mode = edid_cea_modes[vic];
> > unsigned int clock1, clock2;
> > @@ -2956,7 +2964,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode 
> > *to_match)
> > continue;
> >   
> > do {
> > -   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
> > _mode))
> > +   if (drm_mode_match(to_match, _mode, match_flags))
> > return vic;
> > } while (cea_mode_alternate_timings(vic, _mode));
> > }
> > @@ -3003,6 +3011,7 @@ hdmi_mode_alternate_clock(const struct 
> > drm_display_mode *hdmi_mode)
> >   static u8 drm_match_hdmi_mode_clock_tolerance(const struct 
> > drm_display_mode *to_match,
> >   unsigned int clock_tolerance)
> >   {
> > +   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
> >

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-16 Thread Sharma, Shashank 

Regards

Shashank


On 11/13/2017 10:34 PM, Ville Syrjala wrote:

From: Ville Syrjälä 

commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
cause us to not send out any VICs in the AVI infoframes. That commit
was since reverted, but if and when we add aspect ratio handing back
we need to be more careful.

Let's handle this by considering the aspect ratio as a requirement
for cea mode matching only if the passed in mode actually has a
non-zero aspect ratio field. This will keep userspace that doesn't
provide an aspect ratio working as before by matching it to the
first otherwise equal cea mode. And once userspace starts to
provide the aspect ratio it will be considerd a hard requirement
for the match.

Also change the hdmi mode matching to use drm_mode_match() for
consistency, but we don't match on aspect ratio there since the
spec doesn't list a specific aspect ratio for those modes.

Cc: Shashank Sharma 
Cc: "Lin, Jia" 
Cc: Akashdeep Sharma 
Cc: Jim Bride 
Cc: Jose Abreu 
Cc: Daniel Vetter 
Cc: Emil Velikov 
Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/drm_edid.c | 18 ++
  1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7220b8f9a7e8..00aa98f3e55d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
drm_display_mode *mode)
  static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
 unsigned int clock_tolerance)
  {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
  
  	if (!to_match->clock)

return 0;
  
+	if (to_match->picture_aspect_ratio)

+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
This doesn't look right. This means we are expecting a CEA mode without 
a pic aspect ratio field,
which is invalid.  Ankit is going to publish the aspect ratio patch 
series again, with proper DRM cap and flags check. Would it be

ok if we have a look that handling first ?

+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct drm_display_mode cea_mode = edid_cea_modes[vic];
unsigned int clock1, clock2;
@@ -2921,7 +2925,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct 
drm_display_mode *to_m
continue;
  
  		do {

-   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
_mode))
+   if (drm_mode_match(to_match, _mode, match_flags))
return vic;
} while (cea_mode_alternate_timings(vic, _mode));
}
@@ -2938,11 +2942,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const 
struct drm_display_mode *to_m
   */
  u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
  {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
  
  	if (!to_match->clock)

return 0;
  
+	if (to_match->picture_aspect_ratio)

+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;

same here, and probably in other CEA match functions.

+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct drm_display_mode cea_mode = edid_cea_modes[vic];
unsigned int clock1, clock2;
@@ -2956,7 +2964,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode 
*to_match)
continue;
  
  		do {

-   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
_mode))
+   if (drm_mode_match(to_match, _mode, match_flags))
return vic;
} while (cea_mode_alternate_timings(vic, _mode));
}
@@ -3003,6 +3011,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode 
*hdmi_mode)
  static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
  unsigned int clock_tolerance)
  {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
  
  	if (!to_match->clock)

@@ -3020,7 +3029,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const 
struct drm_display_mode *to_
abs(to_match->clock - clock2) > clock_tolerance)
continue;
  
-		if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))

+   if (drm_mode_match(to_match, hdmi_mode, match_flags))
return vic;
}
  
@@ -3037,6 +3046,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_

   */
  static u8

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-13 Thread Ville Syrjälä 
On Mon, Nov 13, 2017 at 06:13:37PM +, Jose Abreu wrote:
> Hi Ville,
> 
> On 13-11-2017 17:04, Ville Syrjala wrote:
> >  
> > +   if (to_match->picture_aspect_ratio)
> > +   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> > +
> >
> 
> Maybe "if (to_match->picture_aspect_ratio !=
> HDMI_PICTURE_ASPECT_NONE && to_match->picture_aspect_ratio !=
> HDMI_PICTURE_ASPECT_RESERVED)"?
> 
> Or some kind of helper to validate the aspect ratio from userspace?

We can't currently get an aspect ratio from userspace. Well we had
it but I reverted it due to regressions. When we add it back it will
obviously need to be validated, just like any other information we
get from userspace.

Hmm. Except we don't even validate the current mode flags/type
at all. Fail.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-13 Thread Jose Abreu 
Hi Ville,

On 13-11-2017 17:04, Ville Syrjala wrote:
>  
> + if (to_match->picture_aspect_ratio)
> + match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> +
>

Maybe "if (to_match->picture_aspect_ratio !=
HDMI_PICTURE_ASPECT_NONE && to_match->picture_aspect_ratio !=
HDMI_PICTURE_ASPECT_RESERVED)"?

Or some kind of helper to validate the aspect ratio from userspace?

Best Regards,
Jose Miguel Abreu
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

2017-11-13 Thread Ville Syrjala 
From: Ville Syrjälä 

commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
cause us to not send out any VICs in the AVI infoframes. That commit
was since reverted, but if and when we add aspect ratio handing back
we need to be more careful.

Let's handle this by considering the aspect ratio as a requirement
for cea mode matching only if the passed in mode actually has a
non-zero aspect ratio field. This will keep userspace that doesn't
provide an aspect ratio working as before by matching it to the
first otherwise equal cea mode. And once userspace starts to
provide the aspect ratio it will be considerd a hard requirement
for the match.

Also change the hdmi mode matching to use drm_mode_match() for
consistency, but we don't match on aspect ratio there since the
spec doesn't list a specific aspect ratio for those modes.

Cc: Shashank Sharma 
Cc: "Lin, Jia" 
Cc: Akashdeep Sharma 
Cc: Jim Bride 
Cc: Jose Abreu 
Cc: Daniel Vetter 
Cc: Emil Velikov 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7220b8f9a7e8..00aa98f3e55d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct 
drm_display_mode *mode)
 static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
 unsigned int clock_tolerance)
 {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
 
if (!to_match->clock)
return 0;
 
+   if (to_match->picture_aspect_ratio)
+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct drm_display_mode cea_mode = edid_cea_modes[vic];
unsigned int clock1, clock2;
@@ -2921,7 +2925,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct 
drm_display_mode *to_m
continue;
 
do {
-   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
_mode))
+   if (drm_mode_match(to_match, _mode, match_flags))
return vic;
} while (cea_mode_alternate_timings(vic, _mode));
}
@@ -2938,11 +2942,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const 
struct drm_display_mode *to_m
  */
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match)
 {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
 
if (!to_match->clock)
return 0;
 
+   if (to_match->picture_aspect_ratio)
+   match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
+
for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) {
struct drm_display_mode cea_mode = edid_cea_modes[vic];
unsigned int clock1, clock2;
@@ -2956,7 +2964,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode 
*to_match)
continue;
 
do {
-   if (drm_mode_equal_no_clocks_no_stereo(to_match, 
_mode))
+   if (drm_mode_match(to_match, _mode, match_flags))
return vic;
} while (cea_mode_alternate_timings(vic, _mode));
}
@@ -3003,6 +3011,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode 
*hdmi_mode)
 static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode 
*to_match,
  unsigned int clock_tolerance)
 {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
 
if (!to_match->clock)
@@ -3020,7 +3029,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const 
struct drm_display_mode *to_
abs(to_match->clock - clock2) > clock_tolerance)
continue;
 
-   if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode))
+   if (drm_mode_match(to_match, hdmi_mode, match_flags))
return vic;
}
 
@@ -3037,6 +3046,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const 
struct drm_display_mode *to_
  */
 static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 {
+   unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | 
DRM_MODE_MATCH_FLAGS;
u8 vic;
 
if (!to_match->clock)
@@ -3052,7 +3062,7 @@ static u8 drm_match_hdmi_mode(const struct 
drm_display_mode *to_match)
 
if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) ||