Virtualization - Feb 2021 - [PATCH v2 10/11] drm/qxl: rework cursor plane

Hi,
> I'm still trying to wrap my head around the qxl cursor code.
> 
> Getting vmap out of the commit tail is good, but I feel like this isn't
> going in the right direction overall.
> 
> In ast, these helper functions were only good when converting the drvier to
> atomic modesetting. So I removed them in the latst patchset and did all the
> updates in the plane helpers directly.
I see the helper functions more as a way to get some structure into the
code flow.  The callbacks are easier to read if they just call helper
functions for stuff which needs more than a handful lines of code
(patch 9/11 exists for the same reason).

The helpers also make it easier move work from one callback to another,
but that is just a useful side-effect.

I had considered making that two separate patches, one factor out code
into functions and one moving the calls.  Turned out to not be that easy
though, because the old qxl_cursor_atomic_update() code was a rather
hairy mix of qxl_create_cursor() + qxl_primary_apply_cursor() +
qxl_primary_move_cursor().
> For cursor_bo itself, it seems to be transitional state that is only used
> during the plane update and crtc update . It should probably be stored in a
> plane-state structure.
> 
> Some of the primary plane's functions seem to deal with cursor
handling.
> What's the role of the primary plane in cursor handling?
It's a quirk.  The qxl device will forget the cursor state on
qxl_io_create_primary(), so I have to remember the cursor state
and re-establish it by calling qxl_primary_apply_cursor() again.

So I'm not sure sticking this into plane state would work.  Because of
the quirk this is more than just a handover from prepare to commit.
> For now, I suggest to merge patch 1 to 8 and 11; and move the cursor
patches
> into a new patchset.
I can merge 1-8, but 11 has to wait until the cursor is sorted.
There is a reason why 11 is last in the series ;)
> I'd like ot hear Daniel's opinion on this. Do you have
> further plans here?
Well.  I suspect I could easily spend a month cleaning up and party
redesign the qxl driver (specifically qxl_draw.c + qxl_image.c).

I'm not sure I'll find the time to actually do that anytime soon.
I have plenty of other stuff on my TODO list, and given that the
world is transitioning to virtio-gpu the priority for qxl isn't
that high.

So, no, I have no short-term plans for qxl beyond fixing pins +
reservations + lockdep.

take care,
  Gerd

Hi

Am 18.02.21 um 12:50 schrieb Gerd Hoffmann:
>    Hi,
> 
>> I'm still trying to wrap my head around the qxl cursor code.
>>
>> Getting vmap out of the commit tail is good, but I feel like this
isn't
>> going in the right direction overall.
>>
>> In ast, these helper functions were only good when converting the
drvier to
>> atomic modesetting. So I removed them in the latst patchset and did all
the
>> updates in the plane helpers directly.
> 
> I see the helper functions more as a way to get some structure into the
> code flow.  The callbacks are easier to read if they just call helper
> functions for stuff which needs more than a handful lines of code
> (patch 9/11 exists for the same reason).
> 
> The helpers also make it easier move work from one callback to another,
> but that is just a useful side-effect.
> 
> I had considered making that two separate patches, one factor out code
> into functions and one moving the calls.  Turned out to not be that easy
> though, because the old qxl_cursor_atomic_update() code was a rather
> hairy mix of qxl_create_cursor() + qxl_primary_apply_cursor() +
> qxl_primary_move_cursor().
> 
>> For cursor_bo itself, it seems to be transitional state that is only
used
>> during the plane update and crtc update . It should probably be stored
in a
>> plane-state structure.
>>
>> Some of the primary plane's functions seem to deal with cursor
handling.
>> What's the role of the primary plane in cursor handling?
> 
> It's a quirk.  The qxl device will forget the cursor state on
> qxl_io_create_primary(), so I have to remember the cursor state
> and re-establish it by calling qxl_primary_apply_cursor() again.
> 
> So I'm not sure sticking this into plane state would work.  Because of
> the quirk this is more than just a handover from prepare to commit.
> 
>> For now, I suggest to merge patch 1 to 8 and 11; and move the cursor
patches
>> into a new patchset.
> 
> I can merge 1-8, but 11 has to wait until the cursor is sorted.
> There is a reason why 11 is last in the series ;)
> 
>> I'd like ot hear Daniel's opinion on this. Do you have
>> further plans here?
> 
> Well.  I suspect I could easily spend a month cleaning up and party
> redesign the qxl driver (specifically qxl_draw.c + qxl_image.c).
> 
> I'm not sure I'll find the time to actually do that anytime soon.
> I have plenty of other stuff on my TODO list, and given that the
> world is transitioning to virtio-gpu the priority for qxl isn't
> that high.
Well, in that case:

Acked-by: Thomas Zimmermann <tzimmermann at suse.de>

for patches 9 and 10. Having the vmap calls fixed is at least worth it.

Best regards
Thomas
> 
> So, no, I have no short-term plans for qxl beyond fixing pins +
> reservations + lockdep.
> 
> take care,
>    Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 N?rnberg, Germany
(HRB 36809, AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210218/b5d1a862/attachment.sig>