| Message ID | ced0d5f5f5b2660e28eed6cf22e8120a0959af90.1495720737.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/25/2017 04:19 PM, Jose Abreu wrote: > Now that we have a callback to check if crtc supports a given mode > we can use it in malidp so that we restrict the number of probbed > modes to the ones we can actually display. > > Also, remove the mode_fixup() callback as this is no longer needed > because mode_valid() will be called before. > > NOTE: Not even compiled tested > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Carlos Palminha <palminha@synopsys.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Brian Starkey <brian.starkey@arm.com> > Cc: David Airlie <airlied@linux.ie> > --- > drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c > index 9446a67..4bb38a2 100644 > --- a/drivers/gpu/drm/arm/malidp_crtc.c > +++ b/drivers/gpu/drm/arm/malidp_crtc.c > @@ -22,9 +22,8 @@ > #include "malidp_drv.h" > #include "malidp_hw.h" > > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > - const struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode) > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc, > + const struct drm_display_mode *mode) > { > struct malidp_drm *malidp = crtc_to_malidp_device(crtc); > struct malidp_hw_device *hwdev = malidp->dev; > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > if (rate != req_rate) { > DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n", > req_rate); > - return false; > + return MODE_NOCLOCK; > } > } > > - return true; > + return MODE_OK; > } > > static void malidp_crtc_enable(struct drm_crtc *crtc) > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, > } > > static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = { > - .mode_fixup = malidp_crtc_mode_fixup, > + .mode_valid = malidp_crtc_mode_valid, > .enable = malidp_crtc_enable, > .disable = malidp_crtc_disable, > .atomic_check = malidp_crtc_atomic_check, > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote: > On 05/25/2017 04:19 PM, Jose Abreu wrote: > > Now that we have a callback to check if crtc supports a given mode > > we can use it in malidp so that we restrict the number of probbed > > modes to the ones we can actually display. > > > > Also, remove the mode_fixup() callback as this is no longer needed > > because mode_valid() will be called before. > > > > NOTE: Not even compiled tested I did compile it, even done some testing, but by no means have I managed to cover all the cases. Looks OK to me. > > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > Cc: Carlos Palminha <palminha@synopsys.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Liviu Dudau <liviu.dudau@arm.com> Acked-by: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Brian Starkey <brian.starkey@arm.com> > > Cc: David Airlie <airlied@linux.ie> > > --- > > drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c > > index 9446a67..4bb38a2 100644 > > --- a/drivers/gpu/drm/arm/malidp_crtc.c > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c > > @@ -22,9 +22,8 @@ > > #include "malidp_drv.h" > > #include "malidp_hw.h" > > > > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > > - const struct drm_display_mode *mode, > > - struct drm_display_mode *adjusted_mode) > > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc, > > + const struct drm_display_mode *mode) > > { > > struct malidp_drm *malidp = crtc_to_malidp_device(crtc); > > struct malidp_hw_device *hwdev = malidp->dev; > > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > > if (rate != req_rate) { > > DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n", > > req_rate); > > - return false; > > + return MODE_NOCLOCK; > > } > > } > > > > - return true; > > + return MODE_OK; > > } > > > > static void malidp_crtc_enable(struct drm_crtc *crtc) > > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, > > } > > > > static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = { > > - .mode_fixup = malidp_crtc_mode_fixup, > > + .mode_valid = malidp_crtc_mode_valid, > > .enable = malidp_crtc_enable, > > .disable = malidp_crtc_disable, > > .atomic_check = malidp_crtc_atomic_check, > > > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote: > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote: > > On 05/25/2017 04:19 PM, Jose Abreu wrote: > > > Now that we have a callback to check if crtc supports a given mode > > > we can use it in malidp so that we restrict the number of probbed > > > modes to the ones we can actually display. > > > > > > Also, remove the mode_fixup() callback as this is no longer needed > > > because mode_valid() will be called before. > > > > > > NOTE: Not even compiled tested > > I did compile it, even done some testing, but by no means have I managed > to cover all the cases. Looks OK to me. > > > > > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > > Cc: Carlos Palminha <palminha@synopsys.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Acked-by: Liviu Dudau <liviu.dudau@arm.com> What does this mean? Do you expect me to merge this through drm-misc? Or do you plan to merge it through your arm tree (all the required patches are in drm-misc-next and will be in Dave's tree soonish)? /me confused. Thanks, Daniel > > > > Cc: Brian Starkey <brian.starkey@arm.com> > > > Cc: David Airlie <airlied@linux.ie> > > > --- > > > drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------ > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c > > > index 9446a67..4bb38a2 100644 > > > --- a/drivers/gpu/drm/arm/malidp_crtc.c > > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c > > > @@ -22,9 +22,8 @@ > > > #include "malidp_drv.h" > > > #include "malidp_hw.h" > > > > > > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > > > - const struct drm_display_mode *mode, > > > - struct drm_display_mode *adjusted_mode) > > > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc, > > > + const struct drm_display_mode *mode) > > > { > > > struct malidp_drm *malidp = crtc_to_malidp_device(crtc); > > > struct malidp_hw_device *hwdev = malidp->dev; > > > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > > > if (rate != req_rate) { > > > DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n", > > > req_rate); > > > - return false; > > > + return MODE_NOCLOCK; > > > } > > > } > > > > > > - return true; > > > + return MODE_OK; > > > } > > > > > > static void malidp_crtc_enable(struct drm_crtc *crtc) > > > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, > > > } > > > > > > static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = { > > > - .mode_fixup = malidp_crtc_mode_fixup, > > > + .mode_valid = malidp_crtc_mode_valid, > > > .enable = malidp_crtc_enable, > > > .disable = malidp_crtc_disable, > > > .atomic_check = malidp_crtc_atomic_check, > > > > > > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
On Wed, May 31, 2017 at 10:20:04AM +0200, Daniel Vetter wrote: > On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote: > > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote: > > > On 05/25/2017 04:19 PM, Jose Abreu wrote: > > > > Now that we have a callback to check if crtc supports a given mode > > > > we can use it in malidp so that we restrict the number of probbed > > > > modes to the ones we can actually display. > > > > > > > > Also, remove the mode_fixup() callback as this is no longer needed > > > > because mode_valid() will be called before. > > > > > > > > NOTE: Not even compiled tested > > > > I did compile it, even done some testing, but by no means have I managed > > to cover all the cases. Looks OK to me. > > > > > > > > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > > > > Cc: Carlos Palminha <palminha@synopsys.com> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > > > Acked-by: Liviu Dudau <liviu.dudau@arm.com> > > What does this mean? Do you expect me to merge this through drm-misc? Or > do you plan to merge it through your arm tree (all the required patches > are in drm-misc-next and will be in Dave's tree soonish)? > > /me confused. /me too. :) I've only got Cc-ed on one patch, so I'm guessing the whole series is going to be picked up through drm-misc. For patches that are part of a larger series (to me) it makes sense to push them through a single channel. But I'm not the author of the series so I don't know what Jose prefers. If Jose wants this patch to go through mali-dp tree then I'm happy to pull it, otherwise I can sort out the conflict(s) before sending a pull request to Dave. On the larger topic, I'm guessing this is not the first time a series touches multiple drivers that are not together in a single tree. How was this sorted in the past? Is there a better way? Best regards, Liviu > > Thanks, Daniel > > > > > > > Cc: Brian Starkey <brian.starkey@arm.com> > > > > Cc: David Airlie <airlied@linux.ie> > > > > --- > > > > drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------ > > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c > > > > index 9446a67..4bb38a2 100644 > > > > --- a/drivers/gpu/drm/arm/malidp_crtc.c > > > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c > > > > @@ -22,9 +22,8 @@ > > > > #include "malidp_drv.h" > > > > #include "malidp_hw.h" > > > > > > > > -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > > > > - const struct drm_display_mode *mode, > > > > - struct drm_display_mode *adjusted_mode) > > > > +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc, > > > > + const struct drm_display_mode *mode) > > > > { > > > > struct malidp_drm *malidp = crtc_to_malidp_device(crtc); > > > > struct malidp_hw_device *hwdev = malidp->dev; > > > > @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, > > > > if (rate != req_rate) { > > > > DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n", > > > > req_rate); > > > > - return false; > > > > + return MODE_NOCLOCK; > > > > } > > > > } > > > > > > > > - return true; > > > > + return MODE_OK; > > > > } > > > > > > > > static void malidp_crtc_enable(struct drm_crtc *crtc) > > > > @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, > > > > } > > > > > > > > static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = { > > > > - .mode_fixup = malidp_crtc_mode_fixup, > > > > + .mode_valid = malidp_crtc_mode_valid, > > > > .enable = malidp_crtc_enable, > > > > .disable = malidp_crtc_disable, > > > > .atomic_check = malidp_crtc_atomic_check, > > > > > > > > > > Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> > > > > -- > > ==================== > > | I would like to | > > | fix the world, | > > | but they're not | > > | giving me the | > > \ source code! / > > --------------- > > ¯\_(ツ)_/¯ > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, May 31, 2017 at 12:48 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > On Wed, May 31, 2017 at 10:20:04AM +0200, Daniel Vetter wrote: >> On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote: >> > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote: >> > > On 05/25/2017 04:19 PM, Jose Abreu wrote: >> > > > Now that we have a callback to check if crtc supports a given mode >> > > > we can use it in malidp so that we restrict the number of probbed >> > > > modes to the ones we can actually display. >> > > > >> > > > Also, remove the mode_fixup() callback as this is no longer needed >> > > > because mode_valid() will be called before. >> > > > >> > > > NOTE: Not even compiled tested >> > >> > I did compile it, even done some testing, but by no means have I managed >> > to cover all the cases. Looks OK to me. >> > >> > > > >> > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> >> > > > Cc: Carlos Palminha <palminha@synopsys.com> >> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> > > > Cc: Liviu Dudau <liviu.dudau@arm.com> >> > >> > Acked-by: Liviu Dudau <liviu.dudau@arm.com> >> >> What does this mean? Do you expect me to merge this through drm-misc? Or >> do you plan to merge it through your arm tree (all the required patches >> are in drm-misc-next and will be in Dave's tree soonish)? >> >> /me confused. > > /me too. :) I've only got Cc-ed on one patch, so I'm guessing the whole series is > going to be picked up through drm-misc. For patches that are part of a larger > series (to me) it makes sense to push them through a single channel. But I'm not > the author of the series so I don't know what Jose prefers. If Jose wants this > patch to go through mali-dp tree then I'm happy to pull it, otherwise I can sort out > the conflict(s) before sending a pull request to Dave. > > On the larger topic, I'm guessing this is not the first time a series touches multiple > drivers that are not together in a single tree. How was this sorted in the past? Is > there a better way? I change my preferred merge strategy depending upon how invasive the patch is. Since this one here is more complex than a simple refactor, I prefer it goes in through the right trees. And the required patches are already in drm-misc-next now, so this should be doable. For simpler stuff it's often easier to just get it landed through drm-misc, especially if it's just a dumb patch to e.g. add a new argument to a function and fill out the default one everywhere. For those I think it's not even required to get an ack from driver maintainers, just solid review of the idea&implementation in general. A bit a grey thing in-between is refactorings that are simple, but require and audit on each driver, and then a final patch at the end to remove the old helper functions. My drm_vblank_cleanup removal is such a case. There I prefer driver maintainers to pick things up themselves, and 1 kernel release afterwards I'll put the leftover driver patches + the final cleanup into drm-misc. Anyway, long story short: Your choice here. I just need to know whether you'll pick it up or want me to merge it through drm-misc-next. I think in general it'd be good if maintainers don't just ack patches, but also state what they expect to happen, e.g. when I ack something I try to make it clear that I expect this to go in through a different tree than one I maintain. Otherwise I just pick it up and merge (and say so). Thanks, Daniel
On Wed, May 31, 2017 at 12:56:59PM +0200, Daniel Vetter wrote: > On Wed, May 31, 2017 at 12:48 PM, Liviu Dudau <liviu.dudau@arm.com> wrote: > > On Wed, May 31, 2017 at 10:20:04AM +0200, Daniel Vetter wrote: > >> On Tue, May 30, 2017 at 10:37:29AM +0100, Liviu Dudau wrote: > >> > On Tue, May 30, 2017 at 09:29:44AM +0200, Neil Armstrong wrote: > >> > > On 05/25/2017 04:19 PM, Jose Abreu wrote: > >> > > > Now that we have a callback to check if crtc supports a given mode > >> > > > we can use it in malidp so that we restrict the number of probbed > >> > > > modes to the ones we can actually display. > >> > > > > >> > > > Also, remove the mode_fixup() callback as this is no longer needed > >> > > > because mode_valid() will be called before. > >> > > > > >> > > > NOTE: Not even compiled tested > >> > > >> > I did compile it, even done some testing, but by no means have I managed > >> > to cover all the cases. Looks OK to me. > >> > > >> > > > > >> > > > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > >> > > > Cc: Carlos Palminha <palminha@synopsys.com> > >> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > > Cc: Liviu Dudau <liviu.dudau@arm.com> > >> > > >> > Acked-by: Liviu Dudau <liviu.dudau@arm.com> > >> > >> What does this mean? Do you expect me to merge this through drm-misc? Or > >> do you plan to merge it through your arm tree (all the required patches > >> are in drm-misc-next and will be in Dave's tree soonish)? > >> > >> /me confused. > > > > /me too. :) I've only got Cc-ed on one patch, so I'm guessing the whole series is > > going to be picked up through drm-misc. For patches that are part of a larger > > series (to me) it makes sense to push them through a single channel. But I'm not > > the author of the series so I don't know what Jose prefers. If Jose wants this > > patch to go through mali-dp tree then I'm happy to pull it, otherwise I can sort out > > the conflict(s) before sending a pull request to Dave. > > > > On the larger topic, I'm guessing this is not the first time a series touches multiple > > drivers that are not together in a single tree. How was this sorted in the past? Is > > there a better way? > > I change my preferred merge strategy depending upon how invasive the > patch is. Since this one here is more complex than a simple refactor, > I prefer it goes in through the right trees. And the required patches > are already in drm-misc-next now, so this should be doable. > > For simpler stuff it's often easier to just get it landed through > drm-misc, especially if it's just a dumb patch to e.g. add a new > argument to a function and fill out the default one everywhere. For > those I think it's not even required to get an ack from driver > maintainers, just solid review of the idea&implementation in general. > > A bit a grey thing in-between is refactorings that are simple, but > require and audit on each driver, and then a final patch at the end to > remove the old helper functions. My drm_vblank_cleanup removal is such > a case. There I prefer driver maintainers to pick things up > themselves, and 1 kernel release afterwards I'll put the leftover > driver patches + the final cleanup into drm-misc. > > Anyway, long story short: Your choice here. I just need to know > whether you'll pick it up or want me to merge it through > drm-misc-next. I think in general it'd be good if maintainers don't > just ack patches, but also state what they expect to happen, e.g. when > I ack something I try to make it clear that I expect this to go in > through a different tree than one I maintain. Otherwise I just pick it > up and merge (and say so). OK, if Jose doesn't like a different approach then I'll pick up this patch. Then I guess I'll keep an eye on when airlied's git tree and see when drm-misc-next gets merged before sending my pull request. And sorry for not stating my follow up action with the Ack, like I've said, I thought the whole series will be picked up by you based on this reply: https://lists.freedesktop.org/archives/dri-devel/2017-May/142377.html Best regards, Liviu > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c index 9446a67..4bb38a2 100644 --- a/drivers/gpu/drm/arm/malidp_crtc.c +++ b/drivers/gpu/drm/arm/malidp_crtc.c @@ -22,9 +22,8 @@ #include "malidp_drv.h" #include "malidp_hw.h" -static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) +static enum drm_mode_status malidp_crtc_mode_valid(struct drm_crtc *crtc, + const struct drm_display_mode *mode) { struct malidp_drm *malidp = crtc_to_malidp_device(crtc); struct malidp_hw_device *hwdev = malidp->dev; @@ -40,11 +39,11 @@ static bool malidp_crtc_mode_fixup(struct drm_crtc *crtc, if (rate != req_rate) { DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n", req_rate); - return false; + return MODE_NOCLOCK; } } - return true; + return MODE_OK; } static void malidp_crtc_enable(struct drm_crtc *crtc) @@ -408,7 +407,7 @@ static int malidp_crtc_atomic_check(struct drm_crtc *crtc, } static const struct drm_crtc_helper_funcs malidp_crtc_helper_funcs = { - .mode_fixup = malidp_crtc_mode_fixup, + .mode_valid = malidp_crtc_mode_valid, .enable = malidp_crtc_enable, .disable = malidp_crtc_disable, .atomic_check = malidp_crtc_atomic_check,
Now that we have a callback to check if crtc supports a given mode we can use it in malidp so that we restrict the number of probbed modes to the ones we can actually display. Also, remove the mode_fixup() callback as this is no longer needed because mode_valid() will be called before. NOTE: Not even compiled tested Signed-off-by: Jose Abreu <joabreu@synopsys.com> Cc: Carlos Palminha <palminha@synopsys.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Brian Starkey <brian.starkey@arm.com> Cc: David Airlie <airlied@linux.ie> --- drivers/gpu/drm/arm/malidp_crtc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)