HDMI Aspect Ratio

[Ville Syrjälä]

On Thu, Oct 19, 2017 at 12:32:53PM -0400, Rob Clark wrote:
> On Wed, Oct 18, 2017 at 4:06 PM, Lloyd Atkinson <latkinso at codeaurora.org> wrote:
> > Hi folks,
> >
> > We're looking at 4K HDMI mode support, and noticed aspect ratio support
> > is in flux.
> >
> > Aspect ratio parsing was added to the mode:
> > https://patchwork.kernel.org/patch/9271401/
> >
> > But later reverted:
> > https://patchwork.kernel.org/patch/9410765/
> >
> > We're finding that since aspect ratio information isn't supported, modes
> > conflict, and the userspace isn't able to see the full list of modes.
> >
> > Similarly, there's a case where two modes differ only by aspect_ratio
> > and vrefresh. In drm_edid.c, within drm_display_mode_from_vic_index,
> > newmode->vrefresh is forcibly being cleared to 0. Since refresh rate is
> > forced to 0, and aspect ratio isn’t considered in general, the modes
> > collide and we can’t advertise one of the modes.
> >
> > https://github.com/torvalds/linux/blob/v4.14-rc4/drivers/gpu/drm/drm_edid.c#L3153
> >
> > Have there been any follow-up discussions on this topic?
> >
> > What is the reason to clear the vrefresh within
> > drm_display_mode_from_vic_index?
> 
> +Ville who might remember the history..
> 
> I don't remember if we found a userspace app that was broken, or if
> reverting was an "abundance of caution" thing (which is equally valid,
> it is basically impossible to track down and test "all of
> userspace"..)

IIRC it was just me being paranoid because we used to have 0 there, and
I was worried something might get confused if it comes in pre-populated.

I don't think this should have any user space impact because the probe
helper will anyway populate it with drm_mode_vrefresh() before we
expose the mode to anyone. So the only time when you might even see the
zero is if you manually duplicate a cea/vic mode for some reason.

So I guess we could just remove the zeroing and see what happens. Or
we might even do something like WARN_ON(vrefresh != drm_mode_vrefresh())
to make sure we've correctly populate the cea/hdmi mode lists.

Note that we really shouldn't be using vrefresh for anything real.
It's there just for informational purposes and you really shouldn't
use it for anything important. drm_mode_equal() & co. ignore it
correctly.

If you have a need to know the actual refresh rate for something
important then 1 Hz accurace seem very much insufficient. We never
use vrefresh for anything in i915 code for instance and instead we
compute things based on the actual timings as needed.

Also I think it's rather unfortunate that the user visible mode even
has vrefresh. It has zero benefits and looks like we have no sanity
checking to make sure the user provided mode vrefresh even agrees
with the rest of the timings. We should probably just set
'mode->vrefresh = drm_mode_vrefres()' in drm_mode_convert_umode() to
make sure it's consistent from the start. Not sure we dare start
rejecting user modes on account of inconsistent vrefresh. That might
really break some userspace applications.

-- 
Ville Syrjälä
Intel OTC