[Spice-devel] [drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Dave Airlie]

On 4 November 2016 at 20:41, Christophe Fergeau  wrote:
> On Thu, Nov 03, 2016 at 06:08:39PM +0100, Gerd Hoffmann wrote:
>> > Or maybe other parts of the
>> > kernel/userspace rely on this rounding down.
>>
>> This is where I suspect we could run in trouble.  Odd resolutions simply
>> don't happen on physical hardware, all usual resolutions are a multiple
>> of 8, most of them are even a multiple of 16.
>>
>> Various image/video formats use 16x16 blocks.
>> The qemu vnc server operates on 16x16 blocks too (and we had bugs in the
>> past with odd resolutions).
>>
>> Also scanlines and cachelines align nicely if you don't use odd
>> resolutions.
>>
>> > I unfortunately don't know
>> > :(
>>
>> I don't have definitive answers too, just a gut feeling that this might
>> cause trouble.
>
> I think this might be fine actually, there is already one such
> resolution in the kernel, which is 1366x768 (1366 is only a multiple of
> 2). There is already a bit of a hack to handle it anyway, see
> fixup_mode_1366x768() in drm_edid.c.
>
>>
>> Maybe we should add a module option for this?  So there is an easy
>> (as-in: doesn't require a kernel rebuild) way out in case it causes
>> trouble in certain setups?
>
> This seems a bit overkill to me, but I can look into it if needed.

I think we should try it an see, if we see problems you could enforce
the framebuffer
would have a stride aligned to whatever value, and just the view into
the framebuffer
could be whatever.

The CVT stuff is just due to how hw is programmed and monitors are described.

Dave.

[Spice-devel] [drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Gerd Hoffmann]

  Hi,

> I think we should try it an see,

Ok, lets try.  I'll go pick them up and prepare a pull with this and
some virtio-gpu bits,

  Gerd

[Spice-devel] [drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

On Thu, Nov 03, 2016 at 06:08:39PM +0100, Gerd Hoffmann wrote:
> > Or maybe other parts of the
> > kernel/userspace rely on this rounding down.
> 
> This is where I suspect we could run in trouble.  Odd resolutions simply
> don't happen on physical hardware, all usual resolutions are a multiple
> of 8, most of them are even a multiple of 16.
> 
> Various image/video formats use 16x16 blocks.
> The qemu vnc server operates on 16x16 blocks too (and we had bugs in the
> past with odd resolutions).
> 
> Also scanlines and cachelines align nicely if you don't use odd
> resolutions.
> 
> > I unfortunately don't know
> > :(
> 
> I don't have definitive answers too, just a gut feeling that this might
> cause trouble.

I think this might be fine actually, there is already one such
resolution in the kernel, which is 1366x768 (1366 is only a multiple of
2). There is already a bit of a hack to handle it anyway, see
fixup_mode_1366x768() in drm_edid.c.

> 
> Maybe we should add a module option for this?  So there is an easy
> (as-in: doesn't require a kernel rebuild) way out in case it causes
> trouble in certain setups?

This seems a bit overkill to me, but I can look into it if needed.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 

[drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Gerd Hoffmann]

On Do, 2016-11-03 at 12:41 +0100, Christophe Fergeau wrote:
> On Thu, Nov 03, 2016 at 09:53:48AM +0100, Gerd Hoffmann wrote:
> > On Mi, 2016-11-02 at 18:00 +0100, Christophe Fergeau wrote:
> > > The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
> > > the resolutions we are going to present to user-space are going to be
> > > rounded down to a multiple of 8. In the QXL arbitrary resolution case,
> > > this is not useful.
> > > This commit forces the actual width/height that was requested by the
> > > client in the drm_display_mode structure rather than keeping the
> > > rounded version.
> > 
> > Hmm, not sure this is a good idea.  There are probably reasons why
> > drm_cvt_mode is rounding down ...
> 
> Yeah, I'm sure there are reasons, but I don't know what they are.
> drm_cvt_mode seems to be calculating various frequencies and timings
> related to refreshing real world monitors, and this seems to be defined
> by some VESA standard. Maybe the rounding down is because the real-world
> monitors or VESA require it.

No worries here, we are in the virtual world, it for sure wouldn't break
monitors ;)

> Or maybe other parts of the
> kernel/userspace rely on this rounding down.

This is where I suspect we could run in trouble.  Odd resolutions simply
don't happen on physical hardware, all usual resolutions are a multiple
of 8, most of them are even a multiple of 16.

Various image/video formats use 16x16 blocks.
The qemu vnc server operates on 16x16 blocks too (and we had bugs in the
past with odd resolutions).

Also scanlines and cachelines align nicely if you don't use odd
resolutions.

> I unfortunately don't know
> :(

I don't have definitive answers too, just a gut feeling that this might
cause trouble.

Maybe we should add a module option for this?  So there is an easy
(as-in: doesn't require a kernel rebuild) way out in case it causes
trouble in certain setups?

cheers,
  Gerd

[drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

On Thu, Nov 03, 2016 at 09:53:48AM +0100, Gerd Hoffmann wrote:
> On Mi, 2016-11-02 at 18:00 +0100, Christophe Fergeau wrote:
> > The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
> > the resolutions we are going to present to user-space are going to be
> > rounded down to a multiple of 8. In the QXL arbitrary resolution case,
> > this is not useful.
> > This commit forces the actual width/height that was requested by the
> > client in the drm_display_mode structure rather than keeping the
> > rounded version.
> 
> Hmm, not sure this is a good idea.  There are probably reasons why
> drm_cvt_mode is rounding down ...

Yeah, I'm sure there are reasons, but I don't know what they are.
drm_cvt_mode seems to be calculating various frequencies and timings
related to refreshing real world monitors, and this seems to be defined
by some VESA standard. Maybe the rounding down is because the real-world
monitors or VESA require it. Or maybe other parts of the
kernel/userspace rely on this rounding down. I unfortunately don't know
:( Any guidance there whether that's ok, or whether I should approach
this differently would be very useful.

Christophe
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: 

[drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Gerd Hoffmann]

On Mi, 2016-11-02 at 18:00 +0100, Christophe Fergeau wrote:
> The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
> the resolutions we are going to present to user-space are going to be
> rounded down to a multiple of 8. In the QXL arbitrary resolution case,
> this is not useful.
> This commit forces the actual width/height that was requested by the
> client in the drm_display_mode structure rather than keeping the
> rounded version.

Hmm, not sure this is a good idea.  There are probably reasons why
drm_cvt_mode is rounding down ...

cheers,
  Gerd

[drm/qxl v2 7/7] qxl: Allow resolution which are not multiple of 8

[Christophe Fergeau]

The use of drm_cvt_mode() in qxl_add_monitors_config_modes() means that
the resolutions we are going to present to user-space are going to be
rounded down to a multiple of 8. In the QXL arbitrary resolution case,
this is not useful.
This commit forces the actual width/height that was requested by the
client in the drm_display_mode structure rather than keeping the
rounded version.

Signed-off-by: Christophe Fergeau 
---
 drivers/gpu/drm/qxl/qxl_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
b/drivers/gpu/drm/qxl/qxl_display.c
index 2f1d738..2241954 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -200,6 +200,9 @@ static int qxl_add_monitors_config_modes(struct 
drm_connector *connector,
mode = drm_cvt_mode(dev, head->width, head->height, 60, false, false,
false);
mode->type |= DRM_MODE_TYPE_PREFERRED;
+   mode->hdisplay = head->width;
+   mode->vdisplay = head->height;
+   drm_mode_set_name(mode);
*pwidth = head->width;
*pheight = head->height;
drm_mode_probed_add(connector, mode);
-- 
2.9.3