Re: [RFC]Dynamic modes support for PV xenfb (included)

[Pat Campbell <plc@novell.com>]

Markus Armbruster wrote:
> Pat Campbell <plc@novell.com> writes:
>   
cut out a whole bunch to address the need to protect some variables.
>
>   
>> +
>> +	xenfb_send_event(info, &event);
>>  }
>>  
>>  static int xenfb_queue_full(struct xenfb_info *info)
>> @@ -209,6 +250,16 @@ static void xenfb_update_screen(struct x
>>  	xenfb_do_update(info, x1, y1, x2 - x1, y2 - y1);
>>  }
>>  
>> +static void xenfb_resize_screen(struct xenfb_info *info)
>> +{
>> +	if (xenfb_queue_full(info))
>> +		return;
>> +
>> +	info->resize_dpy = 0;
>> +	xenfb_do_resize(info);
>> +	xenfb_refresh(info, 0, 0, info->xres, info->yres);
>>     
>
> Hmm, ugly.  See next comment.
>
>   
>> +}
>> +
>>  static int xenfb_thread(void *data)
>>  {
>>  	struct xenfb_info *info = data;
>> @@ -217,6 +268,9 @@ static int xenfb_thread(void *data)
>>  		if (info->dirty) {
>>  			info->dirty = 0;
>>  			xenfb_update_screen(info);
>> +		}
>> +		if (info->resize_dpy) {
>> +			xenfb_resize_screen(info);
>>  		}
>>     
>
> Both screen resizes and dirtying don't go to the backend immediately.
> Instead, they accumulate in struct xenfb_info (dirty rectangle and
> resize_dpy flag) until they get pushed by xenfb_thread().
>
> The way things work, a resize triggers three events:
>
> 1. The update event for the dirty rectangle.  In theory, the dirty
> rectangle could be clean, but I expect it to be quite dirty in
> practice, as user space typically redraws everything right after a
> resize.
>
> 2. The resize event.
>
> 3. Another update event, because xenfb_resize_screen() dirties the
> whole screen.  This event is delayed until the next iteration of
> xenfb_thread()'s loop.
>
> I'd rather do it like this: decree that resize events count as whole
> screen updates.  Make xenfb_resize_screen() clear the dirty rectangle
> (optional, saves an update event).  Test resize_dpy before dirty.
>
> Also: you access resize_dpy, xres, yres and fb_stride from multiple
> threads without synchronization.  I fear that's not safe.  Don't you
> have to protect them with a spinlock, like dirty_lock protects the
> dirty rectangle?  Could reuse dirty_lock, I guess.
>
>   
Don't need to protect these three:
  fb_stride: Eliminated, now using fb_info->fixed.line_len which is read
only after probe()
  xres, yres:
     Set in xenfb_set_par(), read only in all other threads. 

resize_dpy:
While thinking about this variable it occurred to me that a resize would
invalidate any screen updates that are in the queue.  Resize from 
xenfb_thread() was done so that I could ensure that the resize did not
get lost due to a queue full situation. So,  if I clear the queue,
cons=prod, I can add in the resize event packet directly from
xenfb_set_par() getting  rid of resizing from within the thread.  Change
would require a new spinlock to protect the queue but would eliminate
resize_dpy.

Setting cons=prod should be safe.  Worst case is when a clear occurs
while backend is in the event for loop, loop process events it did not
need to. 

xenfb_resize_screen()  becomes:   (called directly from xenfb_set_par())
    spin_lock_irqsave(&info->queue_lock, flags);
    info->page->out_cons = info->page->out_prod;
    spin_unlock_irqrestore(&info->queue_lock, flags);
    xenfb_do_resize(info);
    xenfb_refresh(info, 0, 0, info->xres, info->yres);

xenfb_send_event() becomes:
    spin_lock_irqsave(&info->queue_lock, flags);
    prod = info->page->out_prod;
    /* caller ensures !xenfb_queue_full() */
    mb();           /* ensure ring space available */
    XENFB_OUT_RING_REF(info->page, prod) = *event;
    wmb();          /* ensure ring contents visible */
    info->page->out_prod = prod + 1;
    spin_unlock_irqrestore(&info->queue_lock, flags);

Thoughts on this approach?

Pat