For the avoidance of confusion, I want to make it clear that the latest revisions to the patches in this set posted to the list (v5) address all the review comments from the VPG guys. [v5 1/4] http://patchwork.freedesktop.org/patch/36716/ [2/4] already accepted [v5 3/4] http://patchwork.freedesktop.org/patch/36717/ [v5 4/4] http://patchwork.freedesktop.org/patch/36718/ Thomas. > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf > Of Daniel, Thomas > Sent: Monday, November 17, 2014 2:56 PM > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v5 3/4] drm/i915/bdw: Pin the context > backing objects to GGTT on-demand > > Here is the actual review... > > _____________________________________________ > From: Daniel, Thomas > Sent: Wednesday, November 12, 2014 8:52 PM > To: Goel, Akash > Subject: RE: Execlists patches code review > > > Hi Akash, > > I will put the WARN messages back in and remove the need_unpin. > The elsp_submitted count does not behave exactly as you would expect > because of some race condition. > Have a look at the patch “Avoid non-lite-restore preemptions” by Oscar > Mateo for a description. > > Thanks, > Thomas. > _____________________________________________ > From: Goel, Akash > Sent: Tuesday, November 11, 2014 4:37 PM > To: Daniel, Thomas > Subject: RE: Execlists patches code review > > > Hi Thomas, > > Few comments on http://patchwork.freedesktop.org/patch/35830/ > > int elsp_submitted; > + bool need_unpin; > > This new field has not been used anywhere. > > > if (intel_execlists_ctx_id(ctx_obj) == request_id) { > - WARN(head_req->elsp_submitted == 0, > - "Never submitted head request\n"); > > Sorry couldn’t get this change. Even if a request has been merged, still the > elsp_submitted count should not be 0 here, when this function is executed > on arrival of Context switch interrupt. When a new request is merged with a > previously submitted request, the original value of elsp_submitted is still > retained. > > + /* If the request has been merged, it is possible to > get > + * here with an unsubmitted request. */ > if (--head_req->elsp_submitted <= 0) { > > > > > if (status & GEN8_CTX_STATUS_PREEMPTED) { > if (status & GEN8_CTX_STATUS_LITE_RESTORE) { > - if (execlists_check_remove_request(ring, > status_id)) > - WARN(1, "Lite Restored request > removed from queue\n"); > + execlists_check_remove_request(ring, > status_id); > > Same doubt here, thought that in this case of interrupt due to Preemption > (Lite restore), which will occur when the same Context is submitted as the > one already being executed by the Hw, the count will not drop to 0. Count > will drop to 0 when the context switch interrupt will be generated > subsequently. > > Best regards > Akash > _____________________________________________ > From: Goel, Akash > Sent: Tuesday, November 11, 2014 8:58 PM > To: Daniel, Thomas > Subject: RE: Execlists patches code review > > > Hi Thomas, > > I was OOP today, I will provide this review comment tomorrow on the GFX > mailing list. > > Best regards > Akash > _____________________________________________ > From: Daniel, Thomas > Sent: Monday, November 10, 2014 10:41 PM > To: Goel, Akash > Subject: RE: Execlists patches code review > > > Hi Akash, > > Please post this comment to the mailing list. > Assuming nobody else comments I will remove the unpin_lock and replace > the mutex_lock(&unpin_lock) with WARN_ON(!mutex_is_locked(&dev- > >struct_mutex)). > > Cheers, > Thomas. > > _____________________________________________ > From: Goel, Akash > Sent: Monday, November 10, 2014 11:19 AM > To: Daniel, Thomas > Subject: RE: Execlists patches code review > > > In context of the 3rd patch http://patchwork.freedesktop.org/patch/35829/ > intel_lr_context_pin is being called from logical_ring_alloc_seqno function > and intel_lr_context_unpin gets called from i915_gem_free_request & > i915_gem_reset_ring_cleanup functions > > All these 3 paths are already protected by dev->struct_mutex (Global lock), > so they will always execute sequentially with respect to each other. > > Do we need to have a new lock ? > + struct mutex unpin_lock; > > Best regards > Akash > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
