Re: [Intel-gfx] [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-06 Thread Chris Wilson 
On Thu, Jun 02, 2016 at 11:57:02PM +0200, Daniel Vetter wrote:
> drm_encoder_find is an idr lookup. That should be plenty fast,
> especially for modeset code. Usually what's too expensive even for
> modeset code is linear list walks. But Chris just submitted patches to
> convert most of them into simple lookups.

For the idr_find, I'm tempted to replace the mutex with a rwlock. It
helps pathological cases, but in reality there are more crucial
locks around the hw that limit concurrency. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-03 Thread Daniel Vetter 
On Fri, Jun 03, 2016 at 09:37:43AM +0200, Boris Brezillon wrote:
> On Thu, 2 Jun 2016 23:57:02 +0200
> Daniel Vetter  wrote:
> 
> > On Thu, Jun 2, 2016 at 11:05 PM, Laurent Pinchart
> >  wrote:
> > > Hi Boris,
> > >
> > > Thank you for the patch.
> > >
> > > On Thursday 02 Jun 2016 16:31:28 Boris Brezillon wrote:  
> > >> Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
> > >> drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
> > >> that DRM drivers can leave this hook unassigned if they know they want
> > >> to use drm_atomic_helper_best_encoder().  
> > >
> > > Could you please update include/drm/drm_modeset_helper_vtables.h to 
> > > document
> > > this new behaviour ?  
> > 
> > Thanks for reminding me. Please update hooks for both
> > atomic_best_encoder and best_encoder. Also mention that it's only
> > optional for atomic drivers. There's lots of examples in that file for
> > the wording usually used.
> 
> Hm, I haven't changed anything for the ->atomic_best_encoder() hook, or
> am I missing something?

Before you needed atomic_best_encoder or best_encoder (well that's the
idea at least), now both are optional. Kerneldoc needs to be adjusted in
both places to match new reality after your patch.
-Daniel

> 
> > 
> > > The only drawback I see in this patch is the additional object lookup
> > > performed by drm_atomic_helper_best_encoder() at runtime. I wonder if we 
> > > could
> > > somehow cache the information, as the assignment can't change when 
> > > there's a
> > > 1:1 correspondence between encoders and connectors.  
> > 
> > drm_encoder_find is an idr lookup. That should be plenty fast,
> > especially for modeset code. Usually what's too expensive even for
> > modeset code is linear list walks. But Chris just submitted patches to
> > convert most of them into simple lookups.
> > -Daniel
> > 
> > >>> Signed-off-by: Boris Brezillon   
> > >> ---
> > >>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
> > >>  drivers/gpu/drm/drm_fb_helper.c | 13 -
> > >>  2 files changed, 15 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > >> b/drivers/gpu/drm/drm_atomic_helper.c index f6a3350..849d029 100644
> > >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > >> @@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state
> > >> *state, if (funcs->atomic_best_encoder)
> > >>   new_encoder = funcs->atomic_best_encoder(connector,
> > >>connector_state);
> > >> - else
> > >> + else if (funcs->best_encoder)
> > >>   new_encoder = funcs->best_encoder(connector);
> > >> + else
> > >> + new_encoder = drm_atomic_helper_best_encoder(connector);
> > >>
> > >>   if (!new_encoder) {
> > >>   DRM_DEBUG_ATOMIC("No suitable encoder found for 
> > >> [CONNECTOR:%d:%s]\n",
> > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > >> b/drivers/gpu/drm/drm_fb_helper.c index 7c2eb75..d44389a 100644
> > >> --- a/drivers/gpu/drm/drm_fb_helper.c
> > >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> > >> @@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper
> > >> *fb_helper, my_score++;
> > >>
> > >>   connector_funcs = connector->helper_private;
> > >> - encoder = connector_funcs->best_encoder(connector);
> > >> +
> > >> + /*
> > >> +  * If the DRM device implements atomic hooks and ->best_encoder() 
> > >> is
> > >> +  * NULL we fallback to the default drm_atomic_helper_best_encoder()
> > >> +  * helper.
> > >> +  */
> > >> + if (fb_helper->dev->mode_config.funcs->atomic_commit &&
> > >> + !connector_funcs->best_encoder)
> > >> + encoder = drm_atomic_helper_best_encoder(connector);
> > >> + else
> > >> + encoder = connector_funcs->best_encoder(connector);
> > >> +
> > >>   if (!encoder)
> > >>   goto out;  
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >  
> > 
> > 
> > 
> 
> 
> 
> -- 
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-03 Thread Boris Brezillon 
On Thu, 2 Jun 2016 23:57:02 +0200
Daniel Vetter  wrote:

> On Thu, Jun 2, 2016 at 11:05 PM, Laurent Pinchart
>  wrote:
> > Hi Boris,
> >
> > Thank you for the patch.
> >
> > On Thursday 02 Jun 2016 16:31:28 Boris Brezillon wrote:  
> >> Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
> >> drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
> >> that DRM drivers can leave this hook unassigned if they know they want
> >> to use drm_atomic_helper_best_encoder().  
> >
> > Could you please update include/drm/drm_modeset_helper_vtables.h to document
> > this new behaviour ?  
> 
> Thanks for reminding me. Please update hooks for both
> atomic_best_encoder and best_encoder. Also mention that it's only
> optional for atomic drivers. There's lots of examples in that file for
> the wording usually used.

Hm, I haven't changed anything for the ->atomic_best_encoder() hook, or
am I missing something?

> 
> > The only drawback I see in this patch is the additional object lookup
> > performed by drm_atomic_helper_best_encoder() at runtime. I wonder if we 
> > could
> > somehow cache the information, as the assignment can't change when there's a
> > 1:1 correspondence between encoders and connectors.  
> 
> drm_encoder_find is an idr lookup. That should be plenty fast,
> especially for modeset code. Usually what's too expensive even for
> modeset code is linear list walks. But Chris just submitted patches to
> convert most of them into simple lookups.
> -Daniel
> 
> >>> Signed-off-by: Boris Brezillon   
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
> >>  drivers/gpu/drm/drm_fb_helper.c | 13 -
> >>  2 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> b/drivers/gpu/drm/drm_atomic_helper.c index f6a3350..849d029 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state
> >> *state, if (funcs->atomic_best_encoder)
> >>   new_encoder = funcs->atomic_best_encoder(connector,
> >>connector_state);
> >> - else
> >> + else if (funcs->best_encoder)
> >>   new_encoder = funcs->best_encoder(connector);
> >> + else
> >> + new_encoder = drm_atomic_helper_best_encoder(connector);
> >>
> >>   if (!new_encoder) {
> >>   DRM_DEBUG_ATOMIC("No suitable encoder found for 
> >> [CONNECTOR:%d:%s]\n",
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> >> b/drivers/gpu/drm/drm_fb_helper.c index 7c2eb75..d44389a 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper
> >> *fb_helper, my_score++;
> >>
> >>   connector_funcs = connector->helper_private;
> >> - encoder = connector_funcs->best_encoder(connector);
> >> +
> >> + /*
> >> +  * If the DRM device implements atomic hooks and ->best_encoder() is
> >> +  * NULL we fallback to the default drm_atomic_helper_best_encoder()
> >> +  * helper.
> >> +  */
> >> + if (fb_helper->dev->mode_config.funcs->atomic_commit &&
> >> + !connector_funcs->best_encoder)
> >> + encoder = drm_atomic_helper_best_encoder(connector);
> >> + else
> >> + encoder = connector_funcs->best_encoder(connector);
> >> +
> >>   if (!encoder)
> >>   goto out;  
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >  
> 
> 
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-03 Thread Boris Brezillon 
On Fri, 03 Jun 2016 00:05:46 +0300
Laurent Pinchart  wrote:

> Hi Boris,
> 
> Thank you for the patch.
> 
> On Thursday 02 Jun 2016 16:31:28 Boris Brezillon wrote:
> > Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
> > drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
> > that DRM drivers can leave this hook unassigned if they know they want
> > to use drm_atomic_helper_best_encoder().  
> 
> Could you please update include/drm/drm_modeset_helper_vtables.h to document 
> this new behaviour ?

Sure.

> 
> The only drawback I see in this patch is the additional object lookup 
> performed by drm_atomic_helper_best_encoder() at runtime. I wonder if we 
> could 
> somehow cache the information, as the assignment can't change when there's a 
> 1:1 correspondence between encoders and connectors.
> 
> > Signed-off-by: Boris Brezillon 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
> >  drivers/gpu/drm/drm_fb_helper.c | 13 -
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c index f6a3350..849d029 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state
> > *state, if (funcs->atomic_best_encoder)
> > new_encoder = funcs->atomic_best_encoder(connector,
> >  connector_state);
> > -   else
> > +   else if (funcs->best_encoder)
> > new_encoder = funcs->best_encoder(connector);
> > +   else
> > +   new_encoder = drm_atomic_helper_best_encoder(connector);
> > 
> > if (!new_encoder) {
> > DRM_DEBUG_ATOMIC("No suitable encoder found for 
> > [CONNECTOR:%d:%s]\n",
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c index 7c2eb75..d44389a 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper
> > *fb_helper, my_score++;
> > 
> > connector_funcs = connector->helper_private;
> > -   encoder = connector_funcs->best_encoder(connector);
> > +
> > +   /*
> > +* If the DRM device implements atomic hooks and ->best_encoder() is
> > +* NULL we fallback to the default drm_atomic_helper_best_encoder()
> > +* helper.
> > +*/
> > +   if (fb_helper->dev->mode_config.funcs->atomic_commit &&
> > +   !connector_funcs->best_encoder)
> > +   encoder = drm_atomic_helper_best_encoder(connector);
> > +   else
> > +   encoder = connector_funcs->best_encoder(connector);
> > +
> > if (!encoder)
> > goto out;  
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-02 Thread Daniel Vetter 
On Thu, Jun 2, 2016 at 11:05 PM, Laurent Pinchart
 wrote:
> Hi Boris,
>
> Thank you for the patch.
>
> On Thursday 02 Jun 2016 16:31:28 Boris Brezillon wrote:
>> Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
>> drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
>> that DRM drivers can leave this hook unassigned if they know they want
>> to use drm_atomic_helper_best_encoder().
>
> Could you please update include/drm/drm_modeset_helper_vtables.h to document
> this new behaviour ?

Thanks for reminding me. Please update hooks for both
atomic_best_encoder and best_encoder. Also mention that it's only
optional for atomic drivers. There's lots of examples in that file for
the wording usually used.

> The only drawback I see in this patch is the additional object lookup
> performed by drm_atomic_helper_best_encoder() at runtime. I wonder if we could
> somehow cache the information, as the assignment can't change when there's a
> 1:1 correspondence between encoders and connectors.

drm_encoder_find is an idr lookup. That should be plenty fast,
especially for modeset code. Usually what's too expensive even for
modeset code is linear list walks. But Chris just submitted patches to
convert most of them into simple lookups.
-Daniel

>>> Signed-off-by: Boris Brezillon 
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
>>  drivers/gpu/drm/drm_fb_helper.c | 13 -
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index f6a3350..849d029 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state
>> *state, if (funcs->atomic_best_encoder)
>>   new_encoder = funcs->atomic_best_encoder(connector,
>>connector_state);
>> - else
>> + else if (funcs->best_encoder)
>>   new_encoder = funcs->best_encoder(connector);
>> + else
>> + new_encoder = drm_atomic_helper_best_encoder(connector);
>>
>>   if (!new_encoder) {
>>   DRM_DEBUG_ATOMIC("No suitable encoder found for 
>> [CONNECTOR:%d:%s]\n",
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c index 7c2eb75..d44389a 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper
>> *fb_helper, my_score++;
>>
>>   connector_funcs = connector->helper_private;
>> - encoder = connector_funcs->best_encoder(connector);
>> +
>> + /*
>> +  * If the DRM device implements atomic hooks and ->best_encoder() is
>> +  * NULL we fallback to the default drm_atomic_helper_best_encoder()
>> +  * helper.
>> +  */
>> + if (fb_helper->dev->mode_config.funcs->atomic_commit &&
>> + !connector_funcs->best_encoder)
>> + encoder = drm_atomic_helper_best_encoder(connector);
>> + else
>> + encoder = connector_funcs->best_encoder(connector);
>> +
>>   if (!encoder)
>>   goto out;
>
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-02 Thread Laurent Pinchart 
Hi Boris,

Thank you for the patch.

On Thursday 02 Jun 2016 16:31:28 Boris Brezillon wrote:
> Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
> drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
> that DRM drivers can leave this hook unassigned if they know they want
> to use drm_atomic_helper_best_encoder().

Could you please update include/drm/drm_modeset_helper_vtables.h to document 
this new behaviour ?

The only drawback I see in this patch is the additional object lookup 
performed by drm_atomic_helper_best_encoder() at runtime. I wonder if we could 
somehow cache the information, as the assignment can't change when there's a 
1:1 correspondence between encoders and connectors.

> Signed-off-by: Boris Brezillon 
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
>  drivers/gpu/drm/drm_fb_helper.c | 13 -
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index f6a3350..849d029 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state
> *state, if (funcs->atomic_best_encoder)
>   new_encoder = funcs->atomic_best_encoder(connector,
>connector_state);
> - else
> + else if (funcs->best_encoder)
>   new_encoder = funcs->best_encoder(connector);
> + else
> + new_encoder = drm_atomic_helper_best_encoder(connector);
> 
>   if (!new_encoder) {
>   DRM_DEBUG_ATOMIC("No suitable encoder found for 
> [CONNECTOR:%d:%s]\n",
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> b/drivers/gpu/drm/drm_fb_helper.c index 7c2eb75..d44389a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper
> *fb_helper, my_score++;
> 
>   connector_funcs = connector->helper_private;
> - encoder = connector_funcs->best_encoder(connector);
> +
> + /*
> +  * If the DRM device implements atomic hooks and ->best_encoder() is
> +  * NULL we fallback to the default drm_atomic_helper_best_encoder()
> +  * helper.
> +  */
> + if (fb_helper->dev->mode_config.funcs->atomic_commit &&
> + !connector_funcs->best_encoder)
> + encoder = drm_atomic_helper_best_encoder(connector);
> + else
> + encoder = connector_funcs->best_encoder(connector);
> +
>   if (!encoder)
>   goto out;

-- 
Regards,

Laurent Pinchart

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 01/20] drm/atomic: Fix remaining places where !funcs->best_encoder is valid

2016-06-02 Thread Boris Brezillon 
Adapt drm_pick_crtcs() and update_connector_routing() to fallback to
drm_atomic_helper_best_encoder() if funcs->best_encoder() is NULL so
that DRM drivers can leave this hook unassigned if they know they want
to use drm_atomic_helper_best_encoder().

Signed-off-by: Boris Brezillon 
---
 drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
 drivers/gpu/drm/drm_fb_helper.c | 13 -
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index f6a3350..849d029 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -300,8 +300,10 @@ update_connector_routing(struct drm_atomic_state *state,
if (funcs->atomic_best_encoder)
new_encoder = funcs->atomic_best_encoder(connector,
 connector_state);
-   else
+   else if (funcs->best_encoder)
new_encoder = funcs->best_encoder(connector);
+   else
+   new_encoder = drm_atomic_helper_best_encoder(connector);
 
if (!new_encoder) {
DRM_DEBUG_ATOMIC("No suitable encoder found for 
[CONNECTOR:%d:%s]\n",
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7c2eb75..d44389a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2000,7 +2000,18 @@ static int drm_pick_crtcs(struct drm_fb_helper 
*fb_helper,
my_score++;
 
connector_funcs = connector->helper_private;
-   encoder = connector_funcs->best_encoder(connector);
+
+   /*
+* If the DRM device implements atomic hooks and ->best_encoder() is
+* NULL we fallback to the default drm_atomic_helper_best_encoder()
+* helper.
+*/
+   if (fb_helper->dev->mode_config.funcs->atomic_commit &&
+   !connector_funcs->best_encoder)
+   encoder = drm_atomic_helper_best_encoder(connector);
+   else
+   encoder = connector_funcs->best_encoder(connector);
+
if (!encoder)
goto out;
 
-- 
2.7.4

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization