On Thu, Oct 20, 2016 at 3:19 PM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Thu, Oct 20, 2016 at 03:06:06PM -0400, Rob Clark wrote:
>> It is kind of a pointless restriction. If userspace does silly things
>> like using crtcA's cursor plane on crtcB, and then setcursor on crtcA,
>> it will end up with the overlay disabled on crtcB. But userspace is
>> allowed to shoot itself like this.
>>
>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>> ---
>> Note that cursor and primary planes are slightly useless to non-atomic
>> userspace, since userspace has no way to know *which* crtc a primary
>> or cursor plane belongs to. So lighting up a 2nd display may or may
>> not make your overlay planes go *poof*.
>>
>> So basically use of cursor/primary planes plus legacy ioctls is an
>> undefined behavior if we go with this patch.
>>
>> *Maybe* we want to restrict this to atomic userspace. (Ie. advertise
>> the more limited possible_crtcs for legacy userspace.) Depends on
>> whether we can (slightly retroactively) declare that mixing atomic
>> and legacy APIs is undefined behavior. Or if really needed, add
>> DRM_CLIENT_CAP_REALLY_REALLY_ATOMIC_NO_LEGACY_PLS. (Or just pass in
>> value 2 for existing DRM_CLIENT_CAP_ATOMIC)
>>
>> drivers/gpu/drm/drm_crtc.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index c215802..1e6d37f 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -707,9 +707,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
>> crtc->primary = primary;
>> crtc->cursor = cursor;
>> if (primary)
>> - primary->possible_crtcs = 1 << drm_crtc_index(crtc);
>> + WARN_ON(!(primary->possible_crtcs & (1 << drm_crtc_index(crtc))));
>> if (cursor)
>> - cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
>> + WARN_ON(!(cursor->possible_crtcs & (1 << drm_crtc_index(crtc))));
>
> The slight issue here is that this would require the caller to know the
> upcoming crtc index before its even assigned. So might be better just
> having the caller populate these however it likes after the crtc_init()?
hmm, bleh.. I wanted to make sure we catch drivers that left it as zero..
but how 'bout:
if (cursor && !cursor->possible_crtcs)
cursor->possible_crtcs = 1 << drm_crtc_index(crtc);
BR,
-R
>>
>> if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>> drm_object_attach_property(&crtc->base, config->prop_active, 0);
>> --
>> 2.7.4
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
